< 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
< 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
< 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
< 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>
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
< 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/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.
< 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 :)
< 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
< 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
< 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...
< 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
< 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
< 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