[WIP DONOTMERGE] Replace boost with C++17 (std::shared

MarcoFalke commented at 10:49 pm on June 5, 2020: member

DrahtBot added the label Build system on Jun 5, 2020

DrahtBot added the label Consensus on Jun 5, 2020

DrahtBot added the label GUI on Jun 5, 2020

DrahtBot added the label RPC/REST/ZMQ on Jun 5, 2020

DrahtBot added the label Tests on Jun 5, 2020

DrahtBot added the label Utils/log/libs on Jun 5, 2020

DrahtBot added the label Wallet on Jun 5, 2020

MarcoFalke added the label Refactoring on Jun 5, 2020

MarcoFalke removed the label Build system on Jun 5, 2020

MarcoFalke removed the label Consensus on Jun 5, 2020

MarcoFalke removed the label GUI on Jun 5, 2020

MarcoFalke removed the label RPC/REST/ZMQ on Jun 5, 2020

MarcoFalke removed the label Tests on Jun 5, 2020

MarcoFalke removed the label Utils/log/libs on Jun 5, 2020

MarcoFalke removed the label Wallet on Jun 5, 2020

DrahtBot commented at 0:59 am on June 6, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20736 (rpc: Replace boost::variant with std::variant for RPCArg.m_fallback by MarcoFalke)
#20480 (Replace boost::variant with std::variant by MarcoFalke)
#19288 (fuzz: Add fuzzing harness for TorController by practicalswift)
#19064 (refactor: Cleanup thread ctor calls by hebasto)
#18710 (Add local thread pool to CCheckQueue by hebasto)
#18194 (Bugfix: GUI: Remove broken ability to edit the address field in the sending address book by luke-jr)
#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

MarcoFalke force-pushed on Jun 6, 2020

MarcoFalke force-pushed on Jun 7, 2020

kiminuo commented at 6:56 pm on June 7, 2020: contributor

@MarcoFalke Just an idea - it looks like boost::fs can be replaced with std::filesystem:

The Filesystem library provides facilities for performing operations on file systems and their components, such as paths, regular files, and directories.

The filesystem library was originally developed as boost.filesystem, was published as the technical specification ISO/IEC TS 18822:2015, and finally merged to ISO C++ as of C++17. The boost implementation is currently available on more compilers and platforms than the C++17 library.

https://en.cppreference.com/w/cpp/filesystem

Maybe it’s already on your roadmap. I don’t know. But I can give a hand if you like. (edit: see #19245)

in src/script/sigcache.cpp:30 in 5ce58c3d18 outdated

25@@ -26,7 +26,7 @@ class CSignatureCache
26     CSHA256 m_salted_hasher;
27     typedef CuckooCache::cache<uint256, SignatureCacheHasher> map_type;
28     map_type setValid;
29-    boost::shared_mutex cs_sigcache;
30+    std::shared_mutex cs_sigcache;

JeremyRubin commented at 7:47 pm on June 7, 2020:

note: I think this has little risk of difference with boost vs std since we never use a condition variable with this mutex.

offtopic note for anyone curious is that cs_sigcache is overkill since we generally know that there are no concurrent readers and writers ever, the read lock could be held at a higher layer for the entire span of block validation, it’s just harder to expose the right API for that (@hebasto seems to be looking at this stuff so figured I would mention it somewhere).

JeremyRubin commented at 7:47 pm on June 7, 2020: contributor

concept ACK & lite cr-ACK.

MarcoFalke commented at 11:53 am on June 8, 2020: member

Maybe it’s already on your roadmap. I don’t know. But I can give a hand if you like.

I am not familiar with the filesystem library so I don’t feel qualified to change that code. If you have a patch handy that compiles and works on all our supported operating systems, I am happy to include it here.

laanwj commented at 2:07 pm on June 9, 2020: member

Concept ACK. The changes look good to me.

Maybe it’s already on your roadmap. I don’t know. But I can give a hand if you like.

This would be the eventual goal. However, I think it’d make sense to leave the filesystem change for another PR, as I think it involves quite a lot of ancillary changes to make it work, as well as to be confident it covers everything the current code does (I’m especially a bit afraid with regard to windows and unicode, we’ve had a lot of issues with this in the past).

in src/wallet/rpcwallet.cpp:3005 in 5ce58c3d18 outdated

2965@@ -2966,7 +2966,7 @@ static UniValue listunspent(const JSONRPCRequest& request)
2966             std::unique_ptr<SigningProvider> provider = pwallet->GetSolvingProvider(scriptPubKey);
2967             if (provider) {
2968                 if (scriptPubKey.IsPayToScriptHash()) {
2969-                    const CScriptID& hash = CScriptID(boost::get<ScriptHash>(address));
2970+                    const CScriptID& hash = CScriptID(std::get<ScriptHash>(address));

fanquake commented at 5:45 am on June 14, 2020:

Annoyingly, we can’t do this while we are still supporting macOS 10.12. Using std::get with std::variant is not supported on macOS until 10.14, as that is when the libc++ dylib started supporting the std::bad_variant_access exception. Thus this PR with -mmacos-version-min=10.12 fails to compile:

0wallet/rpcwallet.cpp:2969:60: error: 'get<ScriptHash, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown>' is unavailable: introduced in macOS 10.14
1                    const CScriptID& hash = CScriptID(std::get<ScriptHash>(address));
2                                                           ^
3/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/variant:1376:16: note: 'get<ScriptHash, CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown>' has been explicitly marked unavailable here
4constexpr _Tp& get(variant<_Types...>& __v) {

We could work around this in our code using std::get_if, however it’s still going to be a problem for 3rd-party dependencies. I’ve been testing compiling Qt 5.15 LTS, with -std c++17, and it also requires macOS 10.14+, as they using get with variant throughout cocoa code.

I’ll follow up some discussion in #16684.

MarcoFalke commented at 9:38 am on November 25, 2020:

resolved, now that a52ecc9 is merged

in src/wallet/rpcwallet.cpp:3667 in 5ce58c3d18 outdated

3601@@ -3602,7 +3602,7 @@ class DescribeWalletAddressVisitor : public boost::static_visitor<UniValue>
3602             UniValue subobj(UniValue::VOBJ);
3603             UniValue detail = DescribeAddress(embedded);
3604             subobj.pushKVs(detail);
3605-            UniValue wallet_detail = boost::apply_visitor(*this, embedded);
3606+            UniValue wallet_detail = std::visit(*this, embedded);

fanquake commented at 5:46 am on June 14, 2020:

Similar issue here (as above) using std::visit:

0wallet/rpcwallet.cpp:3605:43: error: 'visit<const DescribeWalletAddressVisitor &, std::__1::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown> &>' is unavailable: introduced in macOS 10.14
1            UniValue wallet_detail = std::visit(*this, embedded);
2                                          ^
3/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/variant:1534:26: note: 'visit<const DescribeWalletAddressVisitor &, std::__1::variant<CNoDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, WitnessUnknown> &>' has been explicitly marked unavailable here
4constexpr decltype(auto) visit(_Visitor&& __visitor, _Vs&&... __vs) {

MarcoFalke commented at 9:38 am on November 25, 2020:

resolved, now that a52ecc9 is merged

fanquake referenced this in commit 3faf3429e9 on Jun 17, 2020

MarcoFalke force-pushed on Jun 19, 2020

in configure.ac:75 in d14b33b70c outdated

71@@ -72,7 +72,7 @@ dnl Require C++11 or C++17 compiler (no GNU extensions)
72 if test "x$use_cxx17" = xyes -o "x$enable_fuzz" = xyes ; then
73   AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory])
74 else
75-  AX_CXX_COMPILE_STDCXX([11], [noext], [mandatory])
76+  AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory])

Sjors commented at 1:48 pm on June 19, 2020:

This if statement is now ridiculous :-) (and comment needs an update)

in .travis.yml:124 in 3d6a5ec9ba outdated

120@@ -121,7 +121,7 @@ jobs:
121         FILE_ENV="./ci/test/00_setup_env_native_tsan.sh"
122 
123     - stage: test
124-      name: 'x86_64 Linux  [GOAL: install]  [bionic]  [no depends, only system libs, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer]'
125+      name: 'x86_64 Linux  [GOAL: install]  [focal]  [no depends, only system libs, sanitizers: address/leak (ASan + LSan) + undefined (UBSan) + integer]'

Sjors commented at 1:49 pm on June 19, 2020:

Maybe bump this Travis machine to focal in a separate PR?

MarcoFalke commented at 1:54 pm on June 19, 2020:

#19267

MarcoFalke force-pushed on Jun 19, 2020

kiminuo referenced this in commit 872b9cf722 on Jun 20, 2020

laanwj referenced this in commit e3fa3c7d67 on Jun 22, 2020

MarcoFalke force-pushed on Jun 29, 2020

MarcoFalke force-pushed on Jul 1, 2020

MarcoFalke force-pushed on Jul 4, 2020

kiminuo referenced this in commit 69ee2dfb69 on Jul 5, 2020

MarcoFalke added this to the milestone 0.22.0 on Aug 2, 2020

kiminuo referenced this in commit fe520b0b5c on Aug 7, 2020

kiminuo referenced this in commit b6d386cd8c on Sep 2, 2020

MarcoFalke force-pushed on Nov 19, 2020

laanwj referenced this in commit 555b5d1bf9 on Nov 23, 2020

practicalswift commented at 7:41 pm on November 23, 2020: contributor

Concept ACK @MarcoFalke What is left to do here until the “[WIP DONTMERGE]” can be dropped? :)

MarcoFalke commented at 8:11 pm on November 23, 2020: member

commit a52ecc9 was required, which was only merged today

hebasto commented at 8:15 pm on November 23, 2020: member

Could 2d483142a7051389afe74c57a216843e6306f1a8 also be reverted?

in src/optional.h:18 in bd7b071ee5 outdated

16-using Optional = boost::optional<T>;
17+using Optional = std::optional<T>;
18 
19 //! Substitute for C++17 std::make_optional
20 template <typename T>
21 Optional<T> MakeOptional(bool condition, T&& value)

practicalswift commented at 8:40 pm on November 23, 2020:

MakeOptional can be dropped if these three workarounds are removed?

0src/wallet/rpcwallet.cpp:    Optional<int> height = MakeOptional(false, int()); // Height of the specified block or the common ancestor, if the block provided was in a deactivated chain.
1src/wallet/rpcwallet.cpp:    Optional<int> stop_height = MakeOptional(false, int());
2src/wallet/wallet.cpp:        Optional<int64_t> time_first_key = MakeOptional(false, int64_t());;

mjdietzx commented at 8:51 pm on November 23, 2020: contributor

Concept ACK

Just an idea - it looks like boost::fs can be replaced with std::filesystem:

and also like this idea

MarcoFalke force-pushed on Nov 24, 2020

practicalswift commented at 10:55 am on November 25, 2020: contributor

@MarcoFalke CI is happily green now. Do you want to make any further changes before removing the draft status, or do you plan to submit these changes as individual PRs?

I’m particularly looking forward to the std::optional change. I’m a bit afraid that mixing boost::optional and std::optional in our code base might lead to some unnecessary confusion.

MarcoFalke commented at 11:05 am on November 25, 2020: member

A green CI doesn’t mean there are no bugs. No one knows if the shared_mutex can be replaced at all: #19183 (review) #16684 (comment)

Also, commit 8b173d4f143c4db4cfe5f88d4fd07e10c146525a introduces a bug in the gui, which isn’t covered by tests.

Finally, optional can’t be replaced with mechanical changes because the standard library doesn’t have a non-throwing accessor by pointer. So the imo confusing use of the non-throwing accessor should be removed first. See #19426

elichai commented at 3:58 pm on November 25, 2020: contributor

Finally, optional can’t be replaced with mechanical changes because the standard library doesn’t have a non-throwing accessor by pointer. So the imo confusing use of the non-throwing accessor should be removed first. See #19426

std::optional acts like a “non-throwing accessor by pointer” by itself, e.g.:

0void print_if(const std::optional<int>& val) {
1    if(val) {
2        std::cout << *val << "\n";
3    } else {
4        std::cout << "empty\n";
5    }
6}

https://godbolt.org/z/jPndjP

MarcoFalke commented at 5:01 pm on November 25, 2020: member

What I meant to say was that it doesn’t have a get_ptr member function. Our current usage would need to be replaced by val.has_value() ? &val.value() : nullptr. So my goal is to first get rid of the get_ptr call.

MarcoFalke force-pushed on Dec 21, 2020

MarcoFalke removed this from the milestone 22.0 on Dec 21, 2020

Use std::shared_mutex 89a4f8dad8

MarcoFalke force-pushed on Jan 12, 2021

MarcoFalke commented at 5:59 am on January 12, 2021: member

Closing for now due to #19183 (review) and #16684 (comment)

MarcoFalke closed this on Jan 12, 2021

MarcoFalke deleted the branch on Jan 12, 2021

MarcoFalke renamed this:
~~[WIP DONOTMERGE] Replace boost with C++17~~
[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex)
on Jan 12, 2021

laanwj referenced this in commit 9996b1806a on Feb 12, 2021

DrahtBot locked this on Aug 16, 2022

[WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) #19183

Conflicts