cmake: set cpp17 standard

[selsta]

It has almost been two years since we switched from C++11 to C++14, last year that we have updated our macOS build SDK which means we could move to C++17.

There's still extensive testing to do with the release binaries before this can be merged.

Any feedback for / against this change?

[jeffro256]

@vtnerd Could c++17's if-constexpr potentially make a lot of the serialization code readable?

[vtnerd]

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

[jtgrassie]

I'm generally against updating base requirements without a really good reason. Bumping the C++ standard to 17 will almost certainly require bumping the minimum version of GCC we support (currently 5). See https://gcc.gnu.org/projects/cxx-status.html#cxx17 for the GCC support.
And I assume that will impact minimum OS build requirements (when using default packaged GCC).

[jeffro256]

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

Sorry lol I meant to type more readable didn't mean to make that sound so rude. Is there really that little tag dispatching in the new lib?

[jeffro256]

I tend to agree with @jtgrassie on this one, though we could replace the boost classes with their new counterparts std::variant and std::optional, etc to reduce dependency on Boost and reduce binary size.

[selsta]

Yes, minimum GCC would increase from 5 to 7. GCC 5 means Ubuntu 16.04 which has been EOL for 2 years now.

Bitcoin Core moved to C++17 2 years ago, but they have the advantage that users can continue to run old versions.

[jtgrassie]

Ubuntu 16.04 which has been EOL for 2 years now

But still on ESM.

And what about FreeBSD etc etc?

I've still yet to see a compelling argument to update.

Bitcoin Core moved to C++17 2 years ago...

Bitcoin whataboutism shouldn't steer the conversation.

[vtnerd]

You think it's unreadable? I think it's relatively straightforward, but then again I wrote it.

And no, constexpr-if won't help much. Afaik, it could only useful in the adaptation for the old style macros. Nothing else does tag dispatching really.

Sorry lol I meant to type more readable didn't mean to make that sound so rude. Is there really that little tag dispatching in the new lib?

I forgot that it's used in the read.h/write.h too. There are overloads for optional vs not optional fields. But the bulk of the code is overloading+adl.

[UkoeHB]

Other than variant/optional, I have not personally encountered anything that made me wish we had C++17. There are a couple minor things in C++20 (std::erase_if(std::unordered_map) and std::popcount).

[tobtoht]

Distro	GCC version	EOL date
Debian 10	8.3.0	2024-06-30
Ubuntu 16.04	5.3 (PPA: 9)	2021-04-02 (EOL)
Ubuntu 18.04	7.3.0 (PPA: 11)	2023-05-31 (EOL)
Fedora 36	12.0.1	2023-05-18 (EOL)
FreeBSD 12	12	2023-12-31
CentOS 7	4.8.5 (SCL: 7)	2024-06-30

I don't think we should take enterprise support programs like Ubuntu's ESM into consideration.

[tobtoht]

std::filesystem might be interesting for new wallet code. Would allow us to eventually drop Boost's filesystem module.

[tobtoht]

Abseil issue

• Monero depends on Protobuf for Trezor support.
• Protobuf depends on Abseil (since version 22) and exposes Abseil's types in its public API.
• Abseil's ABI depends on the C++ version used to compile it. You can't link a project built with C++14 against Abseil built with C++17 and vice versa.
  • Abseil should install w/ the correct base/options.h abseil/abseil-cpp#696 (comment)
• Package maintainers have standardized on using C++17 to build abseil (Arch Linux, Gentoo, Homebrew, Alpine, MSYS2, Fedora)
  • Rationale (Homebrew): abseil: Use the minimum C++ standard supported Homebrew/homebrew-core#133008 (review)
  • See also: [Bug]: Problem using abseil on Macos with C++14 only. abseil/abseil-cpp#1483 (comment)
• Thus, the de facto requirement to include system Protobuf 22+ headers is C++17.
  • This affects: Monero development builds, distro packages and downstream projects that are built from source on systems that ship Protobuf 22+ [1].
  • To enable Trezor support, users/maintainers need to manually set CMAKE_CXX_STANDARD to 17 in CMakeLists.txt.
    • It can't be overridden with -DCMAKE_CXX_STANDARD=17.

Goal: Builds with Trezor support should work without manual intervention with system dependencies installed.

Option 1/n:

• Do not set CMAKE_CXX_STANDARD (allowing CMake to use the compiler's default [2], which matches what we need for Abseil in most cases)
  • Use target_compile_features(monero PUBLIC cxx_std_14) to enforce a minimum of C++14.
  • This does not have the desired effect on macOS, as Apple Clang always defaults to C++98. (Thus CMake would use C++14, instead of C++17 needed for Abseil.)
• Use if(DEPENDS) to force CMAKE_CXX_STANDARD to 14 only for release builds (ensuring no C++17 code makes it into the repo, as these builds are covered by CI).

Option 2/n:

• Add protobuf to ./external and pin it to version 21 and use it if system Protobuf is > 21.
  • This approach adds extra maintenance work.
  • Support for Protobuf 21 ends 2024 Q1. We would need to add both Protobuf and Abseil after this date should a vulnerability be found.
  • Package maintainers would need to use something like -DUSE_SYSTEM_PROTOBUF=On

Option 3/n:

• Merge this PR.
  • All non-EOL distros have a compiler capable of using C++17.
  • We might need to bump the minimum OSX version to 10.14 (EOL since 2021-10-25) if we use C++17 language features.
    • See: macOS: -std=c++17 requires -mmacosx-version-min=10.14 pybind/python_example#44 (comment)
    • Related: depends: use macOS 11 SDK with libc++ headers #8933

Option 4/n:

• Update builds docs for affected distros.
  • A naive cmake .. currently results in 7195 lines of Abseil related compiler warnings (and Trezor support is disabled) - indicating that perhaps something is not quite right.
  • A growing number of devs would be bothered by this as more distros upgrade to Protobuf 22 or later.

Option 5/n:

• (Don't let some random transitive dependency determine which C++ version we build with and kindly ask Trezor to drop Protobuf. jk... unless?)

Option 6/n:

• An elegant solution I haven't thought about. Your input is welcome.

---

[1] Currently: Arch Linux, Alpine Edge, Homebrew, MSYS2.

[2] Compiler defaults

Compiler	Default	Since	Arch Linux	Alpine Edge	Homebrew	MSYS2
GCC	C++17	11 (2021-04-21)	13	13	13	13
Clang	C++17	16 (2023-03-18)	15	x	x	16

[jtgrassie]

Monero depends on Protobuf for Trezor support.

Both of which are optional dependencies. I get the desire to drop supporting older distros, but using optional dependencies as the argument feels a little weak to me. As of today, I can compile and run on Debian Jessie and Ubuntu Xenial with very minimal build flag changes (as I'm sure is the case with older BSDs).

IMO the preferred approach should be to set the minimum std based on the options selected. E.g. if a user wants to build with Trezor support and the minimum std for that is c++17, that then becomes a cmake check to ensure the machines compiler is new enough (or rather simply bump the std when trezor/protobuf is selected).

As it stands right now, we've set a global minimum of c++14, so we know all the required dependencies and Monero source code build with.

But this isn't a hill I'm going to die on! I'd just personally like to see some solid reasons to up the forced minimum. Perhaps our some of our more expert c++ devs (e.g. @vtnerd), could weigh in?

[tobtoht]

Both of which are optional dependencies.

Trezor becomes a required dependency if #8752 is merged.

Currently (without aforementioned PR), Trezor support is enabled by default if its dependencies are found and a test passes, otherwise it fails gracefully and builds continue without Trezor support.

This has lead to a number of cases where Trezor support was unintentionally missing from official releases, Monero packages and downstream projects. From what I recall:

• GUI has shipped without Trezor support
• Arch Linux package ships without Trezor support
  • In one case: Python was missing from the build environment
  • In another: Tests in CheckTrezor.cmake fail with Protobuf 22+
• I have personally shipped Feather Wallet for macOS without Trezor support due to a cross-compilation issue that went unnoticed.

I'm sympathetic towards forcing Trezor support unless explicitly disabled (even just considering time wasted on handling support queries and rebuilds), but with that I do not want development builds to be broken by default on the increasing number of distributions that ship Protobuf 22+. Setting the C++ standard to 17 is IMO the least hacky way to avoid this.

I don't see why security-sensitive software like Monero should guarantee support for distributions that no longer receive security updates. If you have to use an EOL distribution you can install a newer compiler from a PPA or use a release binary if your glibc is recent enough.

[jtgrassie]

Currently (without aforementioned PR), Trezor support is enabled by default if its dependencies are found and a test passes, otherwise it fails gracefully and builds continue without Trezor support.

This is the correct path. If an optional feature's dependencies are not met, it shouldn't be included, regardless if the optional feature is enabled by default.

This has lead to a number of cases where Trezor support was unintentionally missing from official release

This falls in the category of user error (if someone needs Trezor support they should ensure the dependencies are met and the cmake output reports Trezor is being included).

Forcing an update of the c++ std simply for an optional feature is a slippery slope.

I don't see why security-sensitive software like Monero should guarantee support for distributions that no longer receive security updates.

You assume the sysadmin isn't manually addressing security issues. Just because the distribution maintainers no longer "support" doesn't mean the user stops.

As you brought up security-sensitive, a counter argument could also be: Why does Monero even support Trezor given their recently reported cooperation with chainanalysis companies?

If you have to use an EOL distribution you can install a newer compiler...

Of course, but this is digressing from the point. Arguing for bumping the std, globally, to simply support an optional dependency is weak. Especially when the make files can be updated to only bump the std when the user elects to use a feature that requires a certain std (like I mentioned: "or rather simply bump the std when trezor/protobuf is selected").

[SChernykh]

My thoughts on this:

Changing between C++14 and C++17 depending on Trezor disabled/enabled is a bad idea - we can't use C++17 features in this case, so what's the point in having C++17 then?

Today it's Protobuf and Abseil, tomorrow it will be more and more dependencies requiring C++17 (or newer), so we'll have to switch eventually. As correctly pointed out, we must update dependencies from time to time because new versions can have critical fixes.

I did a quick search, and I didn't find any "silent changes" caused by switching to C++17. There's no #ifdef __cplusplus >= 201703L or anything similar to it in the code. So switching to C++17 should be relatively safe, but it will require extensive testing. There can still be some new language features that can change program behavior without giving compilation errors. It needs more checking and testing.

[vtnerd]

@jtgrassie my responses are going to be slanted because I abhor protobuf.

I had basically the same thoughts. We're bumping the minimum compiler version just for trezor/protobuf, which doesn't seem right. As an example, how far do we go - bump to c++20 or newer because the same dependencies require it?

[selsta]

As an example, how far do we go - bump to c++20 or newer because the same dependencies require it?

The compilers we currently use to compile release binaries support C++17, that's why I opened this PR. We can reduce our boost usage and some of the C++17 features might be useful for the new wallet / Seraphis development, or maybe not but at least there's the option. It solving the Abseil issue is a nice side effect.

If a dependency suddenly requires C++20 we will have to find another solution because switching to C++20 is not up for debate.

[ph4r05]

As @SChernykh mentioned, now it is Abseil, but when there is a new vulnerability in a required core dependency, it may happen that security fix will not be available for the old version. When that starts happening, you can either maintain own patches, which is error prone, costly, etc or bump to newer versions, which may require C++17 minimum (it is our for quite some time already). And solution may be needed fast.

I don't have a strong opinion here. Trezor is OK for now, users can still install Protobuf v21 and use that to compile binary, official builds will be compiled also with Protobuf v21. Also the idea with switchable C++17 support sounds good.

You assume the sysadmin isn't manually addressing security issues. Just because the distribution maintainers no longer "support" doesn't mean the user stops.

How long do you wish to support EOL distros in general? I mean, if sysadmin does security patching manually (can, but improbable) they could also install newer compiler, right? 😅

[tobtoht]

Relevant for GUI:

Qt 5.15 is out of support since May 26 and Qt 6 requires C++17.

Qt is still publishing patches for CVEs for 5.15, but it's unclear how long that will last.

[iamamyth]

I don't have a strong opinion on c++17 but this thread brings up an interesting issue: Handing conflicts with rather large third-party components like ledger/trezor. Of course different concrete scenarios may suggest alternate resolutions, but integrating substantial third-party components with large dependency chains into the build tree seems a bit destabilizing. For example, if a new minor release works except for some issue building trezor, is that a blocker, or should there be a policy to only care about these hardware wallets at major releases/hardforks?

[0xFFFC0000]

I am not in favour of switchable C++17 support. As far as I can remember, I haven't seen something like that in any modern code bases (unless I am forgetting something).

But I am very in favour of switching to C++17 by default. Most modern C++ libraries are moving fast right now.

[johnr365]

@vtnerd & @jtgrassie - you've both been vocal in this thread regarding not wanting to move to C++ 17 just to support the Trezor PR/update.

Since then @tobtoht has surfaced an issue with QT 5.15 being out of support since May 26, and Qt 6 requiring C++ 17.

The GUI runs Qt, and thus in the not too distant future may needn Qt 6 (and thus C++ 17) in order to keep getting security patches.

Just wanted to circle back on the discussion to date and see if this new issue with Qt support is enough, in your minds, to justify upgrading to C++ 17.

Doing a quick scan of the thread, it seems that so far, @selsta, @tobtoht, @ph4r05 and @SChernykh are generally in favour of the upgrade, or at least, not opposed to it. (if I'm misrepresenting anyone I apologize, and please correct me).

[SChernykh]

So I was right about it:

Today it's Protobuf and Abseil, tomorrow it will be more and more dependencies requiring C++17 (or newer), so we'll have to switch eventually.

[vtnerd]

The GUI can enable c++17 separately from monero core code (since it's a superset of c++14).

However, I'm not really opposed to the upgrade, as it looks like the relevant platforms support c++17.

[SChernykh]

c++17 is not exactly a superset of c++14 - it removed std::auto_ptr for example, or register keyword: https://en.cppreference.com/w/cpp/keyword/register

I went through changes in c++17 and so far I don't see any changes that would break the program behavior without also breaking code compilation first, so it should be relatively safe to update - but it will need extensive testing.

Edit: There are some changes in expression evaluation order in c++17: https://en.cppreference.com/w/cpp/language/eval_order but they are directed at defining the previously undefined behavior, so it should be also safe - assuming we don't have undefined behavior in the code. If we have it, we must fix it anyway.

[0xFFFC0000]

Keep in mind there are quite silent incompatibilities (and semantic change) between C++14 and C++17 (Annex C [1]). But most of those features are never used. In any case, it wouldn't hurt to double-check.

1. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4713.pdf

[j-berman]

The argument against upgrading for an optional dependency is solid, but, in line with @SChernykh's argument that more dependencies in the future will likely require C++17 (or newer), then it makes sense to get ahead of this and not be on the back foot when the time eventually does come. Ideally we would be prepared to upgrade with minimal disruption.

As of now, there is loose consensus that this PR would need extensive testing before making its way to release. In my view, the best chance to get a switch to C++17 tested on as wide an array of machines as possible is to make the switch on the master branch. Therefore I think it makes sense to move forward with this PR on master, and keep this change out of the release branch until A) it's extensively tested, and B) it's a required change (or at least until stronger reasoning arises to make the switch).

[vtnerd]

Why the changes for locks? We used boost locks because mingw didn't have support - so the windows build might fail now.

[selsta]

@vtnerd compilation failed without the lock change, I don't remember the exact error message but can look it up.

so the windows build might fail now

CI includes mingw Windows builds and everything compiled without issues after this change.

[vtnerd]

The Windows CI hasn't completed yet, did you do something manual?

I would like to see the error, because we use boost::mutex and locks elsewhere too, but those aren't being changed. I don't think there is any harm with mixed usage, but eventually we should move to the platform/std:: locking, if possible.

[selsta]

It was green, I just force pushed to rebase that's why it's running again. Will try to get that error message.

[selsta]

@vtnerd

[ 56%] Building CXX object src/cryptonote_basic/CMakeFiles/obj_cryptonote_format_utils_basic.dir/cryptonote_format_utils_basic.cpp.o
In file included from /home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/locks.hpp:10,
                 from /home/runner/work/monero/monero/src/device/device_ledger.cpp:37:
make[3]: Leaving directory '/home/runner/work/monero/monero/build/arm-linux-gnueabihf/release'
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp: In instantiation of ‘void boost::lock(MutexType1&, MutexType2&) [with MutexType1 = boost::recursive_mutex; MutexType2 = boost::mutex]’:
/home/runner/work/monero/monero/src/device/device_ledger.cpp:567:9:   required from here
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: error: no matching function for call to ‘lock_impl(boost::recursive_mutex&, boost::mutex&, boost::detail::is_mutex_type_wrapper<false>)’
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:144:10: note: candidate: ‘template<class MutexType1, class MutexType2> void boost::detail::lock_impl(MutexType1&, MutexType2&, boost::detail::is_mutex_type_wrapper<true>)’
  144 |     void lock_impl(MutexType1& m1, MutexType2& m2, is_mutex_type_wrapper<true> )
      |          ^~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:144:10: note:   template argument deduction/substitution failed:
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: note:   cannot convert ‘boost::detail::is_mutex_type_wrapper<false>()’ (type ‘boost::detail::is_mutex_type_wrapper<false>’) to type ‘boost::detail::is_mutex_type_wrapper<true>’
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:411:10: note: candidate: ‘template<class Iterator> void boost::detail::lock_impl(Iterator, Iterator, boost::detail::is_mutex_type_wrapper<false>)’
  411 |     void lock_impl(Iterator begin, Iterator end, is_mutex_type_wrapper<false> )
      |          ^~~~~~~~~
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:411:10: note:   template argument deduction/substitution failed:
/home/runner/work/monero/monero/contrib/depends/arm-linux-gnueabihf/include/boost/thread/lock_algorithms.hpp:172:22: note:   deduced conflicting types for parameter ‘Iterator’ (‘boost::recursive_mutex’ and ‘boost::mutex’)
  172 |     detail::lock_impl(m1, m2, detail::is_mutex_type_wrapper<is_mutex_type<MutexType1>::value>());
      |     ~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[3]: *** [src/device/CMakeFiles/obj_device.dir/build.make:118: src/device/CMakeFiles/obj_device.dir/device_ledger.cpp.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:3942: src/device/CMakeFiles/obj_device.dir/all] Error 2

This was for ARMv7 cross compilation.

[vtnerd]

So I guess mingw now supports locks, and maybe threads.

[vtnerd]

I'm ready for C++17; I will immediately make some changes to the new serialization stuff I've been working on.

[jeffro256]

We need this on the release branch now since PRs which depend on c++17 cannot yet be ported to the release branch.