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

[maflcko]

No description provided.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• rpc: Replace boost::variant with std::variant for RPCArg.m_fallback #20736 (rpc: Replace boost::variant with std::variant for RPCArg.m_fallback by MarcoFalke)
• Replace boost::variant with std::variant #20480 (Replace boost::variant with std::variant by MarcoFalke)
• fuzz: Add fuzzing harness for TorController #19288 (fuzz: Add fuzzing harness for TorController by practicalswift)
• refactor: Cleanup thread ctor calls #19064 (refactor: Cleanup thread ctor calls by hebasto)
• Add local thread pool to CCheckQueue #18710 (Add local thread pool to CCheckQueue by hebasto)
• Bugfix: GUI: Remove broken ability to edit the address field in the sending address book #18194 (Bugfix: GUI: Remove broken ability to edit the address field in the sending address book by luke-jr)
• refactor: Extract RipeMd160 #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.

[kiminuo]

@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)

[JeremyRubin]

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

[practicalswift]

Concept ACK

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

[maflcko]

commit a52ecc9 was required, which was only merged today

[hebasto]

Could 2d48314 also be reverted?

[practicalswift]

MakeOptional can be dropped if these three workarounds are removed?

src/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.
src/wallet/rpcwallet.cpp:    Optional<int> stop_height = MakeOptional(false, int());
src/wallet/wallet.cpp:        Optional<int64_t> time_first_key = MakeOptional(false, int64_t());;

[mjdietzx]

Concept ACK

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

and also like this idea

[practicalswift]

@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.

[maflcko]

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

Also, commit 8b173d4 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]

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

void print_if(const std::optional<int>& val) {
    if(val) {
        std::cout << *val << "\n";
    } else {
        std::cout << "empty\n";
    }
}

https://godbolt.org/z/jPndjP

[maflcko]

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.

[maflcko]

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