< bitcoin-git> [bitcoin] sipa pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/bfaed1ab2ec7...88430cbab4dc
< bitcoin-git> bitcoin/master 1e747e3 Mark Friedenbach: Make segwit failure due to CLEANSTACK violation return a SCRIPT_ERR_CLEANSTACK error code.
< bitcoin-git> bitcoin/master 88430cb Pieter Wuille: Merge #12167: Make segwit failure due to CLEANSTACK violation return a SCRIPT_ERR_CLEANSTACK error code...
< bitcoin-git> [bitcoin] sipa closed pull request #12167: Make segwit failure due to CLEANSTACK violation return a SCRIPT_ERR_CLEANSTACK error code (master...cleanstack-script-error) https://github.com/bitcoin/bitcoin/pull/12167
< bitcoin-git> [bitcoin] instagibbs opened pull request #12888: debug log number of unknown wallet records on load (master...unknownrec) https://github.com/bitcoin/bitcoin/pull/12888
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/88430cbab4dc...9a2db3b3d511
< bitcoin-git> bitcoin/master e80c640 John Newbery: [tests] Remove bip9-softforks.py...
< bitcoin-git> bitcoin/master 9c92c8c John Newbery: [tests] Remove Comparison Test Framework
< bitcoin-git> bitcoin/master 9a2db3b MarcoFalke: Merge #11818: I accidentally [deliberately] killed it [the ComparisonTestFramework]...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #11818: I accidentally [deliberately] killed it [the ComparisonTestFramework] (master...remove_comp_framework) https://github.com/bitcoin/bitcoin/pull/11818
< bitcoin-git> [bitcoin] Empact closed pull request #12812: Test ReadConfigFile (master...test-read-config-file) https://github.com/bitcoin/bitcoin/pull/12812
< bitcoin-git> [bitcoin] buddilla opened pull request #12889: doc: add qrencode to brew install instructions (#1) (master...master) https://github.com/bitcoin/bitcoin/pull/12889
< bitcoin-git> [bitcoin] buddilla closed pull request #12889: doc: add qrencode to brew install instructions (#1) (master...master) https://github.com/bitcoin/bitcoin/pull/12889
< wumpus> <jnewbery> wumpus: mind if I take over #7729? <- no, please do
< gribble> https://github.com/bitcoin/bitcoin/issues/7729 | rpc: introduce label API for wallet by laanwj · Pull Request #7729 · bitcoin/bitcoin · GitHub
< wumpus> that PR is cursed :)
< wumpus> aj: could have something to do with how boost test is optimized, from what I remember
< wumpus> some of the tests use BOOST_EQUAL and such in inner loops, while they're somewhat heavyweight
< wumpus> (IIRC, for example one of the verbosity options causes them to log everything)
< wumpus> aj: you could use my methodology from #10026 (need to update it) to find if there are any tests that are especially show with gcc or clang
< gribble> https://github.com/bitcoin/bitcoin/issues/10026 | Overview of slow unit tests · Issue #10026 · bitcoin/bitcoin · GitHub
< aj> wumpus: well, i tried to debug it, but gcc started doing things as fast as clang so now i don't know
< aj> wumpus: better than clang doing things as slow as gcc at least
< wumpus> okay, might have been comparing a debug build against a release build
< wumpus> perf differences are brutal between those
< aj> wumpus: oh, could have been -- do you know which bit is the difference there?
< wumpus> lower optimization level as well as the mutex checking stuff overhead
< aj> wumpus: oh... if anything it seemed that --with-debug was going faster :-/
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/9a2db3b3d511...2fc94370f510
< bitcoin-git> bitcoin/master 5b10ab0 John Newbery: [trivial] Add newlines to end of log messages....
< bitcoin-git> bitcoin/master 2fc9437 Wladimir J. van der Laan: Merge #12887: [trivial] Add newlines to end of log messages....
< bitcoin-git> [bitcoin] laanwj closed pull request #12887: [trivial] Add newlines to end of log messages. (master...log_messages_newlines) https://github.com/bitcoin/bitcoin/pull/12887
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/2fc94370f510...bd59c4395c50
< bitcoin-git> bitcoin/master a5bca13 Luke Dashjr: Bugfix: Include <memory> for std::unique_ptr
< bitcoin-git> bitcoin/master bd59c43 Wladimir J. van der Laan: Merge #12859: Bugfix: Include <memory> for std::unique_ptr...
< bitcoin-git> [bitcoin] laanwj closed pull request #12859: Bugfix: Include <memory> for std::unique_ptr (master...incl_memory) https://github.com/bitcoin/bitcoin/pull/12859
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/bd59c4395c50...2b54155a459c
< bitcoin-git> bitcoin/master a5263fb Indospace.io: doc: Use bitcoind in Tor documentation
< bitcoin-git> bitcoin/master 2b54155 Wladimir J. van der Laan: Merge #12877: doc: Use bitcoind in Tor documentation...
< bitcoin-git> [bitcoin] laanwj closed pull request #12877: doc: Use bitcoind in Tor documentation (master...patch-1) https://github.com/bitcoin/bitcoin/pull/12877
< mryandao> is travis being flaky again?
< wumpus> what's the problem?
< mryandao> #12240 failed due to a timeout for 2
< gribble> https://github.com/bitcoin/bitcoin/issues/12240 | [rpc] Introduced a new `fees` structure that aggregates all sub-field fee types denominated in BTC by mryandao · Pull Request #12240 · bitcoin/bitcoin · GitHub
< mryandao> "The job exceeded the maximum time limit for jobs, and has been terminated."
< wumpus> fantastic
< mryandao> ¯\_ (ツ) _/¯
< wumpus> I've restarted job 2
< aj> i've been trying out the 32bit windows travis checks with more logging; seems like when it succeeds, the coinselection tests take about 15m. i think i'm going to have one fail now, and it's taking ages to compile before even getting to the tests
< mryandao> cant the bitcoin core project afford dedicated machines with gitlab ci?
< aj> wallet/test/coinselector_tests.cpp(17): Leaving test suite "coinselector_tests"; testing time: 950408ms
< wumpus> mryandao: it's more of a problem of people than resources, no one really wants to babysit servers. There used to be a "pulltester" running some CI script on a VM, switching to travis was great. But yeah lately...
< wumpus> I think cfields tried pretty much everything except waving a wad of money in travis representative's face to get more capacity, but no luck
< aj> make is taking 18s on one host, but 822s on the other
< jonasschnelli> there are also the nightly gitian builds I run... though not a real CI (runs no tests)
< jonasschnelli> ideally the gitian builder would test the binaries in some VMs
< aj> (make all i mean, not make check)
< wumpus> jonasschnelli: yes, that would be nice, esp for targets not covered by travis at the moment
< wumpus> aj: yes, compile speed varies a lot between platforms - I suppose memory amount and speed plays a large role, apart from CPU speed
< aj> wumpus: this is the same platform though, i686-w64-mingw32 with DPKG_ADD_ARCH i386
< wumpus> wait, your'e building *on* the same platform or *for* the same platform
< wumpus> or both?
< aj> mostly difference in compile time rather than test time though -- 13m for make all, 20m for test, 11m for make depends, and that's almost the timeout
< aj> wumpus: same job twice, i just rearranged the arg order so it didn't get deduped
< wumpus> ok
< aj> with make check replaced by test_bitcoin.exe -l test_suite or so
< wumpus> your results confuse me a lot
< aj> :)
< aj> i guess it means some of their w64 hosts are overloaded/slow/broken?
< aj> (and also, our coinselector_tests are a bit slow on w64 for some reason? 15 mins seems crazy)
< sipa> that is crazy
< sipa> how fast are they on i686?
< wumpus> so I would guess that's due to boost::test, doing BOOST_EQUALS etc in an inner loop
< aj> haven't tried on other arches; running now as https://travis-ci.org/ajtowns/bitcoin/builds/362613881
< sipa> wumpus: ?
< wumpus> sipa: hm?
< wumpus> oh I can check how long the test takes, if you mean that
< sipa> ah, can we move the inner loop checks out?
< sipa> accumulate a set of failure strings, and thwn assert that it is empty (with the set as message?
< wumpus> yes, that would be a good option
< jnewbery> wumpus: great, I'll have a shot at uncursing it
< wumpus> coinselector_tests takes 10.5 seconds here locally
< wumpus> (linux, x86_64)
< aj> sigh. "test_bitcoin.exe" doesn't work well on non windows funnily enough
< wumpus> what's the problem?
< aj> linux doesn't add ".exe" :)
< wumpus> #8650 "unit tests can be really slow under wine because BOOST_CHECK logs something for all tests. This patch makes them faster by only logging tests which fail. PR'd an alternative to #8632."
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/8632 | Speed up prevector tests by parallelization by JeremyRubin · Pull Request #8632 · bitcoin/bitcoin · GitHub
< wumpus> same problem, but that solution was kind of over the top as it replaces BOOST_CHECK completely, everywhere
< wumpus> so in the end we just sped up the prevector tests instead - coinselector tests likely have the same problem
< wumpus> aj: should still work if wine is installed
< sipa> wumpus: are you sure it's due to coin selection?
< sipa> i'm looking at the total number of assertions in every test, and coin selection is tiny
< sipa> it has 7022 assertions out of 4850320
< wumpus> sipa: that's what aj said - okay in that case it's another thing that is slow in wine
< sipa> coins_tests/updatecoins_simulation_test has 1223073 assertions
< wumpus> whoa
< wumpus> aj ^^
< sipa> what is the command used to actually run the test for windows on travis?
< sipa> wine src/bitcoin/test_bitcoin.exe ?
< wumpus> just 'make check', which will do that indirectly, I expect
< bitcoin-git> [bitcoin] sipa opened pull request #12890: [DEBUG TRAVIS] Detailed unit test report for win32 (master...201803_debugtravis) https://github.com/bitcoin/bitcoin/pull/12890
< sipa> yes but i want to modify the arguments it's being called with
< bitcoin-git> [bitcoin] jnewbery opened pull request #12891: [logging] add lint-logs.sh to check for newline termination. (master...log_lint) https://github.com/bitcoin/bitcoin/pull/12891
< aj> sipa: i've been running "travis_wait 50 src/test/test_bitcoin.exe -l test_suite"
< aj> wallet/test/coinselector_tests.cpp(17): Leaving test suite "coinselector_tests"; testing time: 21124667us # us not ms, so just 21s on x86-64 with qt5
< aj> without the wallet compiled in, all the tests seem to take 33,566,015us so 33s, which seems fine
< aj> and the qt4 test times are reported with units of "mks" so i don't know what that means
< wumpus> apparently, the Russian abbreviation for millisecond...
< ryanofsky> wumpus, had been wondering if you think #10244 is ready, pinged you here: https://github.com/bitcoin/bitcoin/pull/10244#issuecomment-378357905
< gribble> https://github.com/bitcoin/bitcoin/issues/10244 | Refactor: separate gui from wallet and node by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub
< wumpus> ... or microsecond (sorry, the answer is unclear too)
< wumpus> ryanofsky: looks like it!
< aj> sipa: might be faster to build with mingw32 locally, and just include test_bitcoin.exe in the debug commit
< sipa> ?
< aj> sipa: as opposed to actually compiling test_bitcoin.exe on travis, just include it in the PR and run it on travis
< sipa> then i have to make sure the binary is compatible etc
< sipa> hmm, i guess that's not an issue for win binaries
< wumpus> it's indeed not an issue; the windows binaries are self-contained except for OS libs
< sipa> compilation is very slow on travis; don't we use ccache across runs or something?
< wumpus> it should ($HOME/.ccache is among the travis-cached directories)
< sipa> aj: any other explanations about what could make the coin selection test so slow on windows (relatively to other platforms)?
< aj> sipa: i wonder if it's hitting a slow interpretation path in wine somehow?
< wumpus> aj: the way to find out would be to try on real windows
< wumpus> if it's also slow there, gcc-mingw64 is the problem, if not, it's wine
< bitcoin-git> [bitcoin] laanwj pushed 22 new commits to master: https://github.com/bitcoin/bitcoin/compare/2b54155a459c...5f0c6a7b0e47
< bitcoin-git> bitcoin/master ea73b84 Russell Yanofsky: Add src/interface/README.md
< bitcoin-git> bitcoin/master 71e0d90 Russell Yanofsky: Remove direct bitcoin calls from qt/bitcoin.cpp
< bitcoin-git> bitcoin/master c0f2756 Russell Yanofsky: Remove direct bitcoin calls from qt/optionsmodel.cpp
< bitcoin-git> [bitcoin] laanwj closed pull request #10244: Refactor: separate gui from wallet and node (master...pr/ipc-local) https://github.com/bitcoin/bitcoin/pull/10244
< BlueMatt> so we now have 4 pending PRs which rewrite CValidationState, and all of them stomp on each others' toes significantly
< BlueMatt> should probably pick one and get it merged asap instead of waiting until someone *else* decides they also want to rewrite CValidationState
< instagibbs> BlueMatt, clearly we need a standard on how to rewrite CValidationState
< BlueMatt> nono, we need someone else to sit down and *write* a standard, and then a new PR for it
< wumpus> we should get all the people that want to rewrite CValidationState into an (IRC) room and get them to agree what their goal is
< sipa> can we please create a BCIP process first?
< sipa> "bitcoin core implementation proposal"
< wumpus> definitely
< bitcoin-git> [bitcoin] jl2012 closed pull request #8654: [WIP] Reuse sighash computations across evaluation (master...sighashcache) https://github.com/bitcoin/bitcoin/pull/8654
< BlueMatt> wumpus: well it looks like Empact backed out the changes in #12463 that essentially made it into #11639, and #11523 needs rebase pretty significantly (and I'm not a huge fan of parts of it, hence #11639)
< gribble> https://github.com/bitcoin/bitcoin/issues/12463 | Drop the return and corruptionPossible arguments from CValidationState::DoS, and rename to ::Reject by Empact · Pull Request #12463 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/11639 | Rewrite the interface between validation and net_processing wrt DoS by TheBlueMatt · Pull Request #11639 · bitcoin/bitcoin · GitHub
< wumpus> write four BCIPs, then randomly copy/paste from them into one, which is used for the implementation guideline
< gribble> https://github.com/bitcoin/bitcoin/issues/11523 | [Refactor] CValidation State by JeremyRubin · Pull Request #11523 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/11639 | Rewrite the interface between validation and net_processing wrt DoS by TheBlueMatt · Pull Request #11639 · bitcoin/bitcoin · GitHub
< BlueMatt> but #11639 has been open for months and gotten no review except for suhas
< gribble> https://github.com/bitcoin/bitcoin/issues/11639 | Rewrite the interface between validation and net_processing wrt DoS by TheBlueMatt · Pull Request #11639 · bitcoin/bitcoin · GitHub
< sipa> i'll review 11639
< wumpus> would make sense to add it to high priority then, I guess
< sipa> would it help if i concept ack it quickly?
< wumpus> it can't hurt at least
< BlueMatt> it would, yes
< BlueMatt> I can rebase it, but the rebase-needed parts are pretty much in the scripted-diffs, so whatever
< wumpus> for an interface change concepts ack are important
< sipa> if it's clear that that is the approach we want to take, i think we should prioritize the larger-refactor over the patch-up small prs
< wumpus> right
< midnightmagic> '/w 2
< midnightmagic> woops, sorry.
< sipa> unless the larger refactor will take so long regardless that it's better to have small fixes actually in master
< bitcoin-git> [bitcoin] jnewbery opened pull request #12892: [wallet] [rpc] introduce 'label' API for wallet (master...7729_jnewbery) https://github.com/bitcoin/bitcoin/pull/12892
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #7729: rpc: introduce 'label' API for wallet (master...2016_03_wallet_label_api) https://github.com/bitcoin/bitcoin/pull/7729
< jonasschnelli> meeting?
< wumpus> I think in an hour
< jonasschnelli> Oh. I see.
< BlueMatt> Last chance to get reviews in for last week's high-priority list before you get shamed at meeting!
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #12873: [ci] Run functional tests using bitcoin-qt in one Travis job (master...2018-04-03-travis-func-qt) https://github.com/bitcoin/bitcoin/pull/12873
< MarcoFalke> > jonasschnelli ideally the gitian builder would test the binaries in some VMs
< MarcoFalke> I do, if you fix the osx build ;)
< MarcoFalke> windows is WIP
< jonasschnelli> MarcoFalke: is it broken in general or only on my builder (haven't checked)?
< MarcoFalke> jonasschnelli: On your box
< MarcoFalke> There is some network timeout to aws
< MarcoFalke> fetchin a dependency that was recently bumped
< jonasschnelli> I'll have a look.
< MarcoFalke> Awesome, thx
< jonasschnelli> Currently traveling back to CH and back in my office next week
< MarcoFalke> Cool. Wir haben uns schon gefragt wo du so lange bleibst, heh
< MarcoFalke> suggested topics for meeting: Slow unit test/Run unit tests in parallel
< wumpus> good idea
< sipa> yep
< sipa> jonasschnelli: where were you?
< jonasschnelli> sipa: I was 2 month in Indonesia... meeting was 3am... Internet speed was mostly 56kbit (shared with a couple of other people)
< sipa> haha
< sipa> BlueMatt: by "air" you mean "err" ?
< BlueMatt> no? I breathe air
< BlueMatt> oh, you mean on github, no, I mean air, as in "Matt doesn't know AIR-glish"
< sipa> i preferr to err on the side of breathing air over earth
< sipa> *BOOM*
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Apr 5 19:00:20 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< 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
< jonasschnelli> hi
< sipa> hi
< Murch> hi
< jamesob> yo
< 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!).
< phantomcircuit> wat
< wumpus> hi
< meshcollider> Hi
< wumpus> #topic high priority for review
< cfields> hi
< 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.
< 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 :)
< jnewbery> hi
< jamesob> This PR fixes a null pointer deref that's currently in master: https://github.com/bitcoin/bitcoin/pull/12836
< MarcoFalke> ^ Needs rebase
< achow101> hi
< 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!
< instagibbs> achow101, if you rebase psbt I'd nominate it for high prio, not sure you have the time to carry it right now
< wumpus> "Make WalletInitInterface and DummyWalletInit private" really doesn't communicate that
< MarcoFalke> Also, those fixes should go in without having them to put on high-prio
< achow101> instagibbs: I'll try to do that later today or tomorrow
< wumpus> yes, apart from needing rebase it seems to have enough review to go in
< jonasschnelli> indeed
< meshcollider> wumpus: maybe he wasn't aware it fixed that
< wumpus> but please, don't hide fixes in refactor PRs
< kanzure> hi.
< wumpus> meshcollider: right , okay
< jnewbery> I think he wasn't aware of the bug that he fixed when he opened the PR
< wumpus> I see MarcoFalke already improved the title
< wumpus> #topic Slow unit test/Run unit tests in parallel
< cfields> jimpo: thanks for the reviews
< MarcoFalke> I thought that running the unit tests in parallel (similar to how the functional tests are run in parallel) is a free win
< jonasschnelli> MarcoFalke: is that possible with boost?
< MarcoFalke> Seems like they can be parallelized even on a single core
< BlueMatt> yea, most of them use our globals in them
< 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
< BlueMatt> we're a *long way* off from being able to do that, no?
< MarcoFalke> jonasschnelli: I adapted the google parallel tests wrapper
< wumpus> I hope it won't cause any ugly race conditions and such
< wumpus> we have so many intermittent travis failures as is :/
< MarcoFalke> BlueMatt: It works for me
< wumpus> at this point I'd prefer more stable tests to faster ones
< jonasschnelli> AFAIK boost test can't be run in parallel...
< MarcoFalke> at least locally
< MarcoFalke> You spin up different processes of course
< BlueMatt> oh, sorry, i didnt realize they were separate processes, was thinking no way in hell separate threads works
< wumpus> ohh smart
< cfields> jonasschnelli: iirc MarcoFalke's PR creates a wrapper that runs them individually, in parallel
< jnewbery> > I'd prefer more stable tests to faster ones
< jonasschnelli> PR #?
< MarcoFalke> like test_bitcoin -t wallet/t1 & test_bitcoin -t wallet/t2
< jnewbery> We need faster too! Travis PR builds are timing out all over the place
< MarcoFalke> jnewbery: that is a wine issue. Not sure if we can do much about it
< jonasschnelli> Yes. The amount of tests we added during the last year made SAS CI pretty hard
< MarcoFalke> I looked to realize I know not enough of wine to be of any use
< jamesob> not to mention the Travis backlog has been pretty deep lately
< wumpus> jnewbery: I was afraid of some race condition fest, but he spawns multiple processes, so that concern is gone
< achow101> what pr number?
< cfields> #12831
< wumpus> #12831
< gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
< MarcoFalke> Oh Chaincode Labs is willing to sponsor us additional 10 jobs for travis
< MarcoFalke> I hope that goes through until next week
< jonasschnelli> MarcoFalke: nice!
< sipa> pulling in parallel seems like huge overkill though
< jonasschnelli> That's what I just thought
< wumpus> MarcoFalke: nice, but does offering travis more money help? afaik what cfields said, it doens't
< cfields> didn't jeremy start on a replacement for boost_test at one point?
< cfields> yes, I've tried before, but by all means try again!
< wumpus> yes...
< MarcoFalke> wumpus: No, it will only increase the number of jobs
< MarcoFalke> So it clears the backlog faster
< 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.
< MarcoFalke> It doesn't increase the quality or anything
< MarcoFalke> jonasschnelli: No it is a out-of-band update
< wumpus> #8650
< 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
< jonasschnelli> MarcoFalke: Okay. Good to know.
< jamesob> $$$ = more parallelism at the travis job level
< 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?
< MarcoFalke> cfields: I contacted the support
< MarcoFalke> They don't have anything listed on the public website/plans
< cfields> MarcoFalke: huh. I guess it's new then. Great :)
< MarcoFalke> apache or someone did it a few years ago, I am just trying the same
< meshcollider> Cool :)
< wumpus> great
< phantomcircuit> jamesob, i seem to remember the threshold for payed support being better than the free support for oss being pretty high
< sipa> hell yes, go for it
< jonasschnelli> 8650 looks after a huge win.
< MarcoFalke> Doing a wholesale replacement of the test framework seems not a short term solution and perpendicualr to running the tests in parallel
< jtimon> thanks chaincode for the travis jobs!
< wumpus> jtimon: +1
< MarcoFalke> 8650 seems like WIP
< wumpus> MarcoFalke: agree, would be a longer-term concern, if it can be done with boost test that's preferable
< 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.
< wumpus> for now at least
< wumpus> 8650 loses boost test features
< sipa> MarcoFalke: i can't believe that what we need from parallel can't be done with 20 lines of bash
< wumpus> e.g. logging what values mismatch
< wumpus> sipa: yes - just list the test suites, then distribute them over processes
< jonasschnelli> agree
< MarcoFalke> I can't write bash, so someone else has to volunteer
< wumpus> sounds fairly doable in bash, or at least python
< sipa> MarcoFalke: i hereby volunteer
< MarcoFalke> the current thing is python
< aj> 20 lines of python sounds preferable...
< wumpus> python is preferable to me
< wumpus> at least I can help review and maintain it
< BlueMatt> ugh, y'all bash-haters
< MarcoFalke> aj: It has nice features such as a cache for the run times
< sipa> aj hereby volunteered :p
< MarcoFalke> sot the sorting would be done automatically and based on your specs
< sipa> MarcoFalke: ok, 22 lines of bash :)
< jonasschnelli> IMO the whole testing system is already pretty complex. I wouldn't set the burden higher
< MarcoFalke> sipa: Pull requests very welcome :)
< sipa> anyway, i'll see what i can do
< wumpus> ok, so we should look at 12831
< MarcoFalke> And the one sipa proposes
< sipa> #12831
< gribble> https://github.com/bitcoin/bitcoin/issues/12831 | [WIP] Run unit tests in parallel by MarcoFalke · Pull Request #12831 · bitcoin/bitcoin · GitHub
< MarcoFalke> #?????
< MarcoFalke> tba
< jamesob> at what grain does 12831 do parallelism? per file? boost test case?
< MarcoFalke> jamesob: Whatever you like
< sipa> jamesob: one test case per process
< MarcoFalke> Currently ^
< wumpus> per test suite, which is the only parallelism that makes sense
< jonasschnelli> I guess finer (case) would result in concurrency issue
< sipa> no...
< MarcoFalke> ^
< sipa> they're all in separate processes
< jonasschnelli> Have we made sure there are no dependencies between cases?
< sipa> concurrency doesn't even come into the pictire
< wumpus> that sounds like a ton of overhead
< wumpus> launcing a process for every test case
< sipa> wumpus: 250 process creations.
< achow101> cases should be independent of each other
< sipa> ?
< wumpus> yes
< aj> test suite is the file, test case is the function (and each case has many checks)
< jonasschnelli> achow101: Yes. But are they (ex. wallet test)?
< jonasschnelli> But however, suite is what we want not cases
< jonasschnelli> *suites
< cfields> so, the tests can be built as a library...
< MarcoFalke> The savings from --jobs=2 eat all the overhead from running in 250 processes
< wumpus> sounds like a better granularity to me too
< wumpus> in any case we need to get rid of the txt file with all the test cases
< wumpus> and generate that automatically
< meshcollider> Agree
< jonasschnelli> Yes.
< sipa> that seems easy
< jonasschnelli> (same should be done for the functional test IMO, *OT* though9
< wumpus> too easy to forget a test now
< sipa> we can grep for test cases/suites
< MarcoFalke> wumpus: If we keep the list it would be linted on travis of course. *ducks*
< 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?
< 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 :)
< wumpus> MarcoFalke: :-(
< jonasschnelli> +1 :(
< MarcoFalke> Fine
< 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
< wumpus> jtimon: there are some other parameters: sort order, and which list it goes into
< wumpus> jtimon: but anyhow off topic now
< MarcoFalke> Other topics
< jonasschnelli> topic proposal: multiwallet merge (luke-jr brought this up last time while I was not here)
< MarcoFalke> ?
< wumpus> #topic multiwallet GUI
< jonasschnelli> But I guess luke-jr is not here now
< jtimon> right, mhmm, I guess you can rename the tests starting with numbers to keep the order, but that's kind of ugly
< wumpus> cfields: I don't think doing it that way would change the challenges
< jonasschnelli> I heard that the merge of #12610 was done while it was still controversial...
< gribble> https://github.com/bitcoin/bitcoin/issues/12610 | Multiwallet for the GUI by jonasschnelli · Pull Request #12610 · bitcoin/bitcoin · GitHub
< wumpus> jonasschnelli: I'm happy that you merged it
< jonasschnelli> If I did so, my appologies. I only wanted to make progress.
< cfields> ok
< wumpus> if there's anything to be fixed, file a new PR
< jonasschnelli> PRs to fix design mistakes are welcome
< jonasschnelli> yes
< wumpus> luke-jr overblows that part imo
< meshcollider> Yeah it's good that something has been put in, it's been weeks of small disagreement holding it up
< jnewbery> jonasschnelli: better to get it in now so it has time to soak. Rough edges can be fixed in future PRs
< jonasschnelli> I overhauled luke-jr's version mainly because of things like this: https://github.com/bitcoin/bitcoin/pull/11383/files#diff-2c15c5b52f35ea388ebab757eaab0f1cR506
< 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
< wumpus> yes
< jonasschnelli> Yes. I took also care to keep luke-jr authorship in commits during my overhaul.
< jonasschnelli> Okay. Done with that topic then. Thanks
< wumpus> so it's ok, any other topics?
< sipa> let me think
< jnewbery> I had a topic: merge #10244
< jnewbery> but wumpus already did
< gribble> https://github.com/bitcoin/bitcoin/issues/10244 | Refactor: separate gui from wallet and node by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub
< jamesob> woo!
< sipa> i don't have anything
< wumpus> yes, congrats :)
< jnewbery> \o/
< sipa> yay
< jonasschnelli> nice! Yes.
< MarcoFalke> Good to see that in
< BlueMatt> dont forget to review High-Priority PRs this week!
< BlueMatt> </meeting>?
< MarcoFalke> #action dont forget to review High-Priority PRs this week!
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Apr 5 19:32:44 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< aj> any chance of getting #12878 added to high-pri list? it keeps needing rebasing, and at least it's easy to review...
< gribble> https://github.com/bitcoin/bitcoin/issues/12878 | [refactor] Config handling refactoring in preparation for network-specific sections by ajtowns · Pull Request #12878 · bitcoin/bitcoin · GitHub
< cfields> aj: will review asap.
< wumpus> aj: added
< aj> wumpus: thanks
< bitcoin-git> [bitcoin] chivambo opened pull request #12893: junior (master...patch-1) https://github.com/bitcoin/bitcoin/pull/12893
< bitcoin-git> [bitcoin] sipa closed pull request #12893: junior (master...patch-1) https://github.com/bitcoin/bitcoin/pull/12893
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #12894: tests: Avoid test suite name collision in wallet crypto_tests (master...Mf1804-testRenameWalletCryptoTests) https://github.com/bitcoin/bitcoin/pull/12894
< * sipa> learned about xargs -P today
< sipa> cat src/{,wallet/,qt/}test/*.cpp | fgrep BOOST_FIXTURE_TEST_SUITE | cut -d '(' -f 2 | cut -d ',' -f 1 | shuf | xargs -n 1 -P 4 -I "{}" -- ./src/test/test_bitcoin -t "{}/*"
< sipa> MarcoFalke: ^
< achow101> one line of bash?
< MarcoFalke> sipa: Needs white space stripped as well?
< sipa> hmm?
< MarcoFalke> TEST_SUITE( a , ...)
< MarcoFalke> The one before and after "a"
< MarcoFalke> Otherwise I get "Test setup error: no test cases matching filter or all test cases were disabled"
< sipa> heh
< MarcoFalke> That xargs -P is cool
< sipa> add | tr -d '[:space:]' | somewhere
< jamesob> I think `shuf -z` + `xargs -0` is recommended
< MarcoFalke> Also, nice to use one iteration of bogosort to sort by run time
< sipa> jamesob: i don't think we're particularly worried about test names with newlines in them :)
< sipa> but yes
< jamesob> sipa: I'll make it a point to include one in my next testcase ;)
< MarcoFalke> Someone should see if it works with emojis in test names
< * sipa> checks C++ standard
< jamesob> I mean heck, if we're gonna do this as a one-liner we can at least overengineer it a little
< webuser323> sipa, MarcoFalke, I was just reading meeting log and though maybe 'man parallel' since you mentioned .sh for running tests in parallel. It's a great little tool.
< sipa> webuser323: it is, and MarcoFalke wanted to use that initially
< sipa> unfortunately it's a bit of a hassle.to include it in our repo
< webuser323> no problems, just thought I'll chime in with this in case it slipped somehow
< sipa> thanks!
< bitcoin-git> [bitcoin] practicalswift opened pull request #12895: tests: Add note about test suite name uniqueness requirement to developer notes (master...check-uniqueness-of-test-suite-names) https://github.com/bitcoin/bitcoin/pull/12895
< wumpus> you could always abuse make to run things parallel *ducks*
< aj> wumpus: nice
< sipa> wumpus: oh!
< sipa> do you know how make does thread counting across processes?
< sipa> it's so cute
< achow101> instagibbs: rebased psbt
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #12894: tests: Avoid test suite name collision in wallet crypto_tests (master...Mf1804-testRenameWalletCryptoTests) https://github.com/bitcoin/bitcoin/pull/12894
< wumpus> sipa: no, I don't know
< cfields> sipa: the special fd thing?
< sipa> wumpus: it has a shared fifo; every time a thread starts, it reads a char from that fifo, and when that is done, it writes another character
< sipa> it doesn't know or care where that char goes :p
< wumpus> ahh that makes sense, nice, didn't know you could share a fifo that way
< cfields> sipa: feel free to hack that into test_runner so that we could hook it up to make and -jX would just work as intended :)
< cfields> that's been on my personal todo for ages
< instagibbs> achow101, great
< bitcoin-git> [bitcoin] practicalswift opened pull request #12896: docs: Fix conflicting statements about initialization in developer notes (master...remove-conflicing-statements) https://github.com/bitcoin/bitcoin/pull/12896
< bitcoin-git> [bitcoin] practicalswift closed pull request #12782: Explicitly state our assumptions about LookupBlockIndex(...) return values (master...LookupBlockIndex-assumptions) https://github.com/bitcoin/bitcoin/pull/12782
< bitcoin-git> [bitcoin] practicalswift opened pull request #12897: Add GetBlockIndex(const uint256& hash) for when the caller assumes that the block index exists for the given block hash (master...GetBlockIndex) https://github.com/bitcoin/bitcoin/pull/12897