< bitcoin-git> [bitcoin] andrewtoth closed pull request #19271: validation: Warm coins cache during prevalidation to connect blocks faster (master...warm-coinscache) https://github.com/bitcoin/bitcoin/pull/19271
< sipa> i don't understand this
< sipa> it's failing consistently, in the same way, for all platforms that run functional tests & have wallet enabled
< sipa> but i can't reproduce it locally, not even after git clean -dfx && ccache -C
< sipa> can anyone try to build & run #17977, and run tests/functional/feature_taproot.py ?
< gribble> https://github.com/bitcoin/bitcoin/issues/17977 | Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa · Pull Request #17977 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] JeremyRubin closed pull request #19531: add static_check_equal for easier to read compiler errors (master...static_check) https://github.com/bitcoin/bitcoin/pull/19531
< bitcoin-git> [bitcoin] JeremyRubin reopened pull request #19531: add static_check_equal for easier to read compiler errors (master...static_check) https://github.com/bitcoin/bitcoin/pull/19531
< sipa> oh, maybe a recent merge
< troygiorshev> sipa: test passed for me
< sipa> troygiorshev: thanks for trying - i think i know what caused it
< sipa> the merge of #19620
< gribble> https://github.com/bitcoin/bitcoin/issues/19620 | Add txids with non-standard inputs to reject filter by sdaftuar · Pull Request #19620 · bitcoin/bitcoin · GitHub
< troygiorshev> ah great
< troygiorshev> fwiw too, I think this test should be moved to the "less than 2m" section
< troygiorshev> Took 1:12 on my i7-8750H
< aj> sipa: that seems to match the error in the travis log
< sipa> troygiorshev: thanks, doing that
< sipa> aj: yeah, i can reproduce after a rebase on master
< fanquake> sipa: gotta keep up :p
< fanquake> bitcoin/bitcoin is a fast moving repository
< sipa> fanquake: i started this set of changes over a week ago, then when i was done rebased on master, and right then "someone" merges probably the only open PR in the whole repo that breaks the tests without causing a compilation failure :p
< bitcoin-git> [bitcoin] JeremyRubin opened pull request #19677: WIP: Switch BlockMap to use an unordered_set under the hood (master...refactor-blockmap-blockset) https://github.com/bitcoin/bitcoin/pull/19677
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/6d8543504d8c...4b705b1c98f6
< bitcoin-git> bitcoin/master edc3160 Russell Yanofsky: test: Remove duplicate NodeContext hacks
< bitcoin-git> bitcoin/master 4b705b1 MarcoFalke: Merge #19098: test: Remove duplicate NodeContext hacks
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #19098: test: Remove duplicate NodeContext hacks (master...pr/qtx) https://github.com/bitcoin/bitcoin/pull/19098
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #15045: [test] Apply maximal flags to tx_valid tests and minimal flags to tx_invalid tests (master...min_txtests_flags) https://github.com/bitcoin/bitcoin/pull/15045
< sdaftuar> sipa: just catching up now, is the test in #19620 not stable?
< gribble> https://github.com/bitcoin/bitcoin/issues/19620 | Add txids with non-standard inputs to reject filter by sdaftuar · Pull Request #19620 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] fanquake closed pull request #19675: Run clang-tidy -*,performance-* (master...master) https://github.com/bitcoin/bitcoin/pull/19675
< sipa> sdaftuar: no no; there was an actual interaction between 19620 and 17977, and 17977's tests started failing after merging (as it didn't move taproot outputs out of WITNESS_UNKNOWN, those ended up being nonstandard to spend)
< sdaftuar> ah!
< bitcoin-git> [bitcoin] sdaftuar opened pull request #19680: 0.20: Add txids with non-standard inputs to reject filter (0.20...2020-08-reject-unknown-wit-0.20) https://github.com/bitcoin/bitcoin/pull/19680
< bitcoin-git> [bitcoin] sdaftuar opened pull request #19681: 0.19: Add txids with non-standard inputs to reject filter (0.19...2020-08-reject-unknown-wit-0.19) https://github.com/bitcoin/bitcoin/pull/19681
< bitcoin-git> [bitcoin] hebasto closed pull request #19600: doc: Fix log message for -reindex (master...200727-load-log) https://github.com/bitcoin/bitcoin/pull/19600
< bitcoin-git> [bitcoin] dongcarl opened pull request #19683: WIP: depends: Explicitly specify clang search paths for darwin host (master...2020-08-clang-search-path-robustness) https://github.com/bitcoin/bitcoin/pull/19683
< bitcoin-git> [bitcoin] JeremyRubin closed pull request #18071: Refactoring CHashWriter & Get{Prevouts,Sequence,Outputs}Hash to SHA256 (master...refactoring-hashers) https://github.com/bitcoin/bitcoin/pull/18071
< Kiminuo> Hi, I'm trying to slowly move forward this https://github.com/bitcoin/bitcoin/pull/19245. And it seems I need to put somewhere `-lstdc++fs` (as per https://www.bfilipek.com/2019/05/boost-to-stdfs.html) to fix failing travis configurations (https://travis-ci.org/github/bitcoin/bitcoin/builds/715745829). I know everybody is busy but any advice or pointer would help me immensely as this is not something I know much about. :-|
< jeremyrubin> hi Kiminuo
< sipa> Kiminuo: is that needed if you're actually building in c++17 mode?
< jeremyrubin> I don't think you need to do -lstdc++fs if the gcc > 9.0
< jeremyrubin> It sounds like you only need it to build against an earlier version
< Kiminuo> well, I can see that #include <filesystem> is not recognized in those tests
< sipa> -l only affects linking
< sipa> it can't affect header includes
< Kiminuo> I'm not entirely sure c++17 is correctly enabled in my branch
< Kiminuo> ok
< sipa> do you build with --enable-c++17 ?
< jeremyrubin> The travis builds won't work until c++17 is default I guess
< jeremyrubin> This patch might just have to sit for a year for that
< Kiminuo> https://github.com/bitcoin/bitcoin/pull/19245/files#diff-309426131fa2cf2ebbc5cde58961b8a9R19 - Marco Falke actually have a PR that I build upon
< Kiminuo> and I can see a few instance of `--enable-c++17`
< Kiminuo> *instances
< jeremyrubin> you need to pass it to ./configure --enable-c++17
< Kiminuo> jeremyrubin, I'm fine with waiting :)
< jeremyrubin> sipa may know otherwise, but I suspect waiting till we are no longer building for c++11 is the most likely case due to the API changes
< gwillen> the change to configure.ac in that PR does appear to make c++17 unconditional in configure
< gwillen> it removes the configure flag and forces it on
< gwillen> (I don't know if there could be other places that were using it that got missed though)
< gwillen> right, yeah. So I'm not sure if $use_cxx17 is referenced anywhere else, or if that change is enough to be the same as passing the flag to configure.
< gwillen> I don't see any other uses, so off the top of my head I'd expect that to work
< Kiminuo> Well, travis shows:
< Kiminuo> ./fs.h:14:10: fatal error: filesystem: No such file or directory
< Kiminuo> #include <filesystem>
< Kiminuo> ^~~~~~~~~~~~
< gwillen> looking at the travis failures it does look like some headers got rearranged in c++17 that you haven't accounted for, but I don't know if you were even looking at that yet (e.g. std::function got moved into #include <functional>, which is causing a ton of errors, but not the ones you're asking about)
< sipa> Kiminuo: but your changes to use std::filesystem instead of boost::filesystem should only be active whenever you're building in C++17 mode
< sipa> i think?
< sipa> ah
< sipa> sorry, just reading
< jeremyrubin> sipa: they're building on mandatory c++17
< Kiminuo> gwillen, I have based my PR on this: https://github.com/bitcoin/bitcoin/pull/19183
< sipa> hmm, ok
< sipa> and does the problem arise on all platforms?
< gwillen> configure does say at the end: CXX = /usr/bin/ccache g++ -m64 -std=c++17
< Kiminuo> sipa, all I know is this result: https://travis-ci.org/github/bitcoin/bitcoin/builds/715745829
< Kiminuo> > but your changes to use std::filesystem instead of boost::filesystem should only be active whenever you're building in C++17 mode
< sipa> yeah, ignore me;
< Kiminuo> that would be preferable I guess, but it's out of scope of my ability
< sipa> erhhh
< gwillen> it seems that the <filesystem> library may not be broadly available yet, even in c++17 mode
< sipa> the windows build fails to include <filesystem>
< sipa> that sounds like we just can't use it
< sipa> even in c++17 mode
< gwillen> it's hard for me to find up-to-date info though
< Kiminuo> Interestingly, I have a C++ project on Windows where <filesystem> works.
< gwillen> it seems likely there is a minimum compiler version to support it
< jeremyrubin> Kiminuo: I would just wait on it till mandatory c++17 goes in
< jeremyrubin> it should be like a year or so
< sipa> jeremyrubin: that won't help
< sipa> because c++17 is enabled in these builds that are failing
< jeremyrubin> sipa: wait on it meaning, not worth spending more time now?
< gwillen> what version of gcc do we use? It appears <filesystem> is introduced without qualifications in gcc 8
< gwillen> before that you can use <experimental/filesystem> with an additional compile flag
< sipa> jeremyrubin: waiting on mandatory c++17 won't change the fact that these builds fail
< sipa> oh
< sipa> ok, i get it
< sipa> i assumed that -std=c++17 would only _work_ at all on compilers that fully support c++17
< gwillen> yeah it seems that it means "give me as much c++17 as you have"
< sipa> right
< jeremyrubin> the shakespeare rule
< sipa> so we won't make c++17 mandatory until we drop support for all platforms that don't support it fully (or at least, sufficiently)
< jeremyrubin> Or, we will do that
< jeremyrubin> as long as we have libs we can still use
< Kiminuo> https://travis-ci.org/github/bitcoin/bitcoin/jobs/715745835 - I can see in this build that GCC 7.5 is used. Is enough for <filesystem>?
< jeremyrubin> like boost::filesystem
< sipa> how so?
< gwillen> Kiminuo: if you still want to experiment, understanding it could be quite awhile before this is feasible in production, it looks like you could try including <experimental/filesystem> and linking with "-lstdc++fs"
< gwillen> Kiminuo: no it seems you need 8.0
< Kiminuo> gwillen, so that may be the issue, right?
< gwillen> yeah I think that is indeed the issue
< jeremyrubin> I think c++17 will be fine for even "as much as you have of 17" compilers with the current code
< jeremyrubin> but we may not be able to use *all* c++17 features until then
< gwillen> your local compiler is newer than the one that CI is using, so you have <filesystem> but CI does not
< gwillen> I assume the compiler CI uses is the benchmark for "minimum supported to build bitcoin with"?
< Kiminuo> gwillen, I don't think <experimental/filesystem> is way to go. It will make it more complex and it does not sound good to use something experimental in Bitcoin.
< gwillen> I believe it is the same library, it just moved when it was finally standardized to a new name. But I agree it may not be suitable for inclusion into Bitcoin for real for now.
< gwillen> especially since boost::filesystem is probably _also_ the same library under a different name, more or less
< Kiminuo> std::filesystem is kind of renamed boost::filesystem with some modifications, but the details are sometimes not that easy to find (at least for me)
< gwillen> yeah, that's usually how it goes with C++ library features being standardized I think -- they start in boost and work their way into the standard library
< sipa> GCC 7 jusy predates the c++17 standard being finalized
< sipa> so i guess that'z why it's only available under experimental - at the time it was written there was no way to know of that would eventually match the spec
< sipa> GCC 7.5 is more than recent enough, but i guess these things are only updated in major releasesm
< Kiminuo> https://travis-ci.org/github/bitcoin/bitcoin/jobs/715745836 - but here GCC is also 7.5 and the test passes
< sipa> hmm, strange
< Kiminuo> actually it's verbatim the same thing as in the failing test
< Kiminuo> https://www.diffchecker.com/Q52OO5NI - "Build system information" sections are actually the same for OK test & failing test.
< gwillen> according to https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2017 it was not added until 8.1 _and_ it still requires a special link argument, -lstdc++fs
< gwillen> so, there's something weird happening
< Kiminuo> yeah, I have mentioned that earlier
< Kiminuo> (that link argument)
< Kiminuo> I'm not sure how long it would take to wait for GCC 8.1 to be widespread.
< sipa> this is something to report on https://github.com/bitcoin/bitcoin/issues/16684
< sipa> so perhaps the right thing to do is making std::filesystem based builds still configurable, and only enable them on GCC 8.1+ platforms and others that we know support it
< sipa> so that the lack of full std::filesystem support doesn't hold up otherwise migrating to C++17
< sipa> gwillen, Kiminuo: perhaps a discrepancy is due to libstdc++ and gcc not being the exact same thing and available in lockstep?
< sipa> std::filesystem support is purely a libstdc++ side thing, i expect
< Kiminuo> sipa, I don't know about that configuration. The APIs of both libraries is not in one-to-one correspondence. It can be done by a wrapper I guess.
< sipa> well, that wrapper already exists
< sipa> this is why we have the fs namespace
< Kiminuo> True
< Kiminuo> But you would need to add more functions
< sipa> i expect that slight differences between boost::filesystem and std::filesystem make it not exactly a "using fs = boost::filesystem" vs "using fs = std::filesystem", but it shouldn't be too much more than that
< Kiminuo> https://github.com/bitcoin/bitcoin/pull/19245 - it's documented here
< Kiminuo> The differences are not that big
< sipa> ok
< Kiminuo> But if you have a fs::path and you use operator "/", I think it behaves slightly differently. And that may be hard to explain to people
< sipa> what is the difference?
< Kiminuo> there was something like this
< Kiminuo> let me check what that was actually about
< Kiminuo> PR 19466 is based on boost::filesystem and Marco said that in https://github.com/bitcoin/bitcoin/pull/19466#discussion_r451809243 you would get `/tmp//sadf` and in std::filesystem you would get "/foo"
< Kiminuo> I hope I'm saying that correctly
< sipa> heh
< Kiminuo> That's pretty nasty, if true
< Kiminuo> That makes me thing that using boost::filesystem & std::filesystem at the same time may bring troubles