< bitcoin-git> [bitcoin] AkioNak opened pull request #14919: test: Prevent "Duplicate-wallet filename specified" (master...confirm_unloadwallet_done) https://github.com/bitcoin/bitcoin/pull/14919
< jimpo> thanks for the reviews, jamesob!
< jamesob> jimpo: more to come. I want me some block filters!
< bitcoin-git> [bitcoin] Empact opened pull request #14920: Build: enable -Wdocumentation via isystem (master...isystem) https://github.com/bitcoin/bitcoin/pull/14920
< meshcollider> provoostenator: would you mind re-reviewing #14646 if you have some time
< gribble> https://github.com/bitcoin/bitcoin/issues/14646 | Add expansion cache functions to descriptors (unused for now) by sipa · Pull Request #14646 · bitcoin/bitcoin · GitHub
< fanquake> wumpus only 2 signed sigs? Or haven't you pushed yours up
< wumpus> yes, I still have to upload mine
< wumpus> thanks for remindingm e
< fanquake> np
< provoostenator> I'm rebuilding my unsigned ones first because it was complaining about "cp: cannot stat 'inputs/bitcoin-0.17.1rc1-win-unsigned.tar.gz"
< provoostenator> (just copied a neweer version of gitian-build.py
< provoostenator> Happy to review 13646 again. Can someone remind me of the git incantation to see what changed relative to my 94677cc utACK? (assuming I still have it locally)
< wumpus> if you just want to see the difference since then: git diff 94677cc
< wumpus> this will work okay if either commits were added since then, or commits were squashed/reorganized in place. As it is the total diff that includes the difference in base commits it's not useful to review a rebase.
< wumpus> for reviewing a rebase*
< fanquake> Chris_Stewart_5 is rapidcheck work essentially blocked on upstream?
< Chris_Stewart_5> fanquake: I haven't had time to track down the root cause. Does it appear that memory access violation is happening in rapidcheck?
< fanquake> Chris_Stewart_5 Yes I think so. The bitcoin related test which is failing seems too simple? to be the cause.
< bitcoin-git> [bitcoin] ken2812221 opened pull request #14922: windows: Set_WIN32_WINNT to 0x0601 (Windows 7) (master...patch-1) https://github.com/bitcoin/bitcoin/pull/14922
< Chris_Stewart_5> Yes I think it must be. I guess i could remove the test since all of the other ones seems to pass and we can merge that PR.
< promag> Does it make sense dynamic maxtxfee?
< promag> currently it requires a restart?
< wumpus> do we still support windows vista or not? I'm confused (re: #14922)
< gribble> https://github.com/bitcoin/bitcoin/issues/14922 | windows: Set _WIN32_WINNT to 0x0600 (Windows Vista) by ken2812221 · Pull Request #14922 · bitcoin/bitcoin · GitHub
< wumpus> apparently Randolf removed mention of it from the compatibility notes in the release notes
< wumpus> in #12546
< gribble> https://github.com/bitcoin/bitcoin/issues/12546 | [docs] Minor improvements to Compatibility Notes by randolf · Pull Request #12546 · bitcoin/bitcoin · GitHub
< wumpus> for lack of testing on that version I think it makes sense, but, well, I don't think we've ever discussed it
< promag> vista is not supported by microsoft since april 2017
< wumpus> ok
< wumpus> which suggests that we shouldn't either
< fanquake> I'd be surprised if anyone is testing on vista at all.
< promag> even win7?
< fanquake> qt 5.12 will still support win7, so I think we should try and keep compat for now
< promag> yap
< fanquake> So we should only support Windows 10?
< fanquake> and essentially > 1709
< wumpus> no, I'd say we should support win7
< wumpus> many people are still using that
< fanquake> Potentially larger issue is qts removal of support for "old" macOS versions. 5.12 is 10.12+
< promag> we are far from min qt 5.12
< fanquake> It doesn't have to be min, just being able to use in depends
< fanquake> I'd like to try and use 5.12 (LTS) for 0.18.0
< promag> it fixes some macos issues AFAIK
< fanquake> and backport the last 5.9.x (LTS) point release to 0.17.x
< Chris_Stewart_5> fanquake: Sorry lost connection, thoughts on just removing that test for now? Like you said it is a very simple test
< promag> provoostenator: added new screenshot to #14573
< gribble> https://github.com/bitcoin/bitcoin/issues/14573 | qt: Add Window menu by promag · Pull Request #14573 · bitcoin/bitcoin · GitHub
< promag> provoostenator: I'll push in a bit
< fanquake> Chris_Stewart_5 possibly, if you just want to be able to merge #14853 (assuming no other tests fail) in a similar way. I can test later tonight.
< gribble> https://github.com/bitcoin/bitcoin/issues/14853 | [wip] depends: latest rapidcheck, enable property based tests on Travis by fanquake · Pull Request #14853 · bitcoin/bitcoin · GitHub
< fanquake> When I tried compiling #14430 I ran into some other, potentially related, issues.
< gribble> https://github.com/bitcoin/bitcoin/issues/14430 | Add more property based tests for basic bitcoin data structures by Christewart · Pull Request #14430 · bitcoin/bitcoin · GitHub
< Chris_Stewart_5> fanquake: I believe the issues with #14430 where from not having the EXTRAS module installed from rapidcheck? At least I believe that was MarcoFalke's problem with #14171
< gribble> https://github.com/bitcoin/bitcoin/issues/14430 | Add more property based tests for basic bitcoin data structures by Christewart · Pull Request #14430 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/14171 | travis: Run property based testing by MarcoFalke · Pull Request #14171 · bitcoin/bitcoin · GitHub
< fanquake> Chris_Stewart_5 I've tried compiling ontop of the new PR, and was seeing compilation problems, some output here: https://github.com/bitcoin/bitcoin/pull/14430#issuecomment-443400827
< Chris_Stewart_5> fanquake: Actually looking closer it appears there are two different properties failing between 14171 and 14430 'key_sign_symmetry' and 'key_set_symmetry'
< Chris_Stewart_5> Interesting, i missed that comment
< Chris_Stewart_5> did we bump to c++14?
< wumpus> Chris_Stewart_5: no
< fanquake> Chris_Stewart_5 No, maybe for 0.19.0 #13356
< gribble> https://github.com/bitcoin/bitcoin/issues/13356 | RFC: C++14 Requirement · Issue #13356 · bitcoin/bitcoin · GitHub
< fanquake> If anyones interested, while doing rc1 testing, I'll be adding bitcoin-core vm setups to a Vagrantfile here: https://github.com/fanquake/core-review/tree/master/vagrant
< fanquake> A lot of my vm setup/testing had been pretty manual using virtualbox, so I've decided to try and automate it further.
< fanquake> For ex, make it's fairly easy to github-merge a PR, then "spin up" an openBSD vm, with core dependencies/debugging tools already available, and have the code sitting inside.
< Chris_Stewart_5> That can be used for more generalized testing, i.e. rapid check stuff right? Is there much of a difference between this and using docker?
< provoostenator> wumpus: thanks. Indeed I want to review the rebase, which involves slightly more magic
< fanquake> Chris_Stewart_5 Yea. I did use Docker directly to recreate Travis for the rapidcheck debugging, but decided to use vagrant here, as it sits "above" virtualbox, docker etc
< Chris_Stewart_5> Interesting. I probably won't get around to testing until this weekend but I'll give it a try.
< provoostenator> Stack Overflow to the rescue: git commit-tree HEAD^{tree} -p 94677cc -p HEAD~5 -m "Rebase changes since 94677cc" && git show
< provoostenator> Where 5 is the number of commits in the PR
< provoostenator> (neh, I don't think that worked, I'll leave a comment on the PR once I find it)
< provoostenator> Ah, it's: git show ` git commit-tree ...`
< fanquake> Chris_Stewart_5 I've just pushed up a travis vm definition: https://github.com/fanquake/core-review/commit/26032d6bdcdf7e928f8ce22c97e29c9846b39d59
< fanquake> If you do a github-merge 14853, then vagrant up travis-linux-no-depends-qt
< fanquake> At the end you can ssh into the box, and run gdb --args src/test/test_bitcoin --log_level=all --run_test=key_properties
< fanquake> then "run", "thread apply bt all" and it'll give you the backtrace you're after
< wumpus> fanquake: that seems very useful, I'm still mostly using manual testing in VMs too, can be a hassle sometimes especially when the VMs need to be upgraded, would be good to automate
< wumpus> provoostenator: good to know
< provoostenator> wumpus: actually that didn't work either. I left a git range-diff incantation on the PR, though the diff is too big to be useful.
< wumpus> provoostenator: what I usually do is manually repeat the rebase (on a temporary branch), then compare against the author's, it's a lot of hassle though especially the rebase doesn't go cleanup
< wumpus> cleanly*
< wumpus> I'd hoped there was a cleaner/faster way but seems not
< provoostenator> git range-diff `git merge-base --all HEAD 94677cc `... 94677cc HEAD~5...HEAD
< provoostenator> Though it takes Matrix level reading skills to understand the result often.
< fanquake> wumpus I added some example usage docs, https://github.com/fanquake/core-review/tree/master/vagrant#testing-pull-requests , will add more vms this week.
< provoostenator> I annotated a range-diff for a simpler commit #14573, though I suspect there's a better way...
< gribble> https://github.com/bitcoin/bitcoin/issues/14573 | qt: Add Window menu by promag · Pull Request #14573 · bitcoin/bitcoin · GitHub
< meshcollider> If anyone has some time, #14886 review would be very appreciated so we can get it merged, a lot depends on it
< gribble> https://github.com/bitcoin/bitcoin/issues/14886 | [tests] Refactor importmulti tests by jnewbery · Pull Request #14886 · bitcoin/bitcoin · GitHub
< promag> can I have #14573 in HP?
< gribble> https://github.com/bitcoin/bitcoin/issues/14573 | qt: Add Window menu by promag · Pull Request #14573 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #14925: test: Correctly deserialize without witness (master...Mf1812-testWitnessDeser) https://github.com/bitcoin/bitcoin/pull/14925
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #14926: test: consensus: Check that final transactions are valid (master...Mf1812-testConsensusFinal) https://github.com/bitcoin/bitcoin/pull/14926
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/5f23460c7e31...d680ef9381b4
< bitcoin-git> bitcoin/master 522b80b qubenix: add `--retry 5` to curl opts in install_db4.sh
< bitcoin-git> bitcoin/master d680ef9 MarcoFalke: Merge #14883: add `--retry 5` to curl opts in install_db4.sh...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #14883: add `--retry 5` to curl opts in install_db4.sh (master...qubenix-curl-retry) https://github.com/bitcoin/bitcoin/pull/14883
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/d680ef9381b4...7701b62561c4
< bitcoin-git> bitcoin/master 0dcac51 Gregory Sanders: wallet_keypool_topup.py: Test for all keypool address types
< bitcoin-git> bitcoin/master 7701b62 MarcoFalke: Merge #14857: wallet_keypool_topup.py: Test for all keypool address types...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #14857: wallet_keypool_topup.py: Test for all keypool address types (master...keypool_topup_addresstype) https://github.com/bitcoin/bitcoin/pull/14857
< bitcoin-git> [bitcoin] MarcoFalke pushed 8 new commits to master: https://github.com/bitcoin/bitcoin/compare/7701b62561c4...f65bce858f26
< bitcoin-git> bitcoin/master cb41ade John Newbery: [tests] fix flake8 warnings in wallet_importmulti.py
< bitcoin-git> bitcoin/master e5a8ea8 John Newbery: [tests] tidy up imports in wallet_importmulti.py
< bitcoin-git> bitcoin/master 7c99614 John Newbery: [tests] add get_key function to wallet_importmulti.py...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #14886: [tests] Refactor importmulti tests (master...importmulti_tests) https://github.com/bitcoin/bitcoin/pull/14886
< bitcoin-git> [bitcoin] gmaxwell opened pull request #14929: Allow connections from misbehavior banned peers. (master...201812-softbanmisbehaving) https://github.com/bitcoin/bitcoin/pull/14929
< meshcollider> promag: done
< promag> thanks meshcollider