Obliterate Boost

[vasild]

• While it may be possible to refactor the code in a way that it does
  not use any optional types, like in a616312, fb73b81, 138ad67,
  5724a2c, that would be error prone and would require bigger changes.
  Switch from C++14 to C++17 instead and replace boost::optional with
  std::optional.

• Removing boost::current_exception_diagnostic_information() - if the
  caught exception is an instance of std::exception, use its what()
  method. Otherwise don't provide extra diagnostic information. After
  all boost::current_exception_diagnostic_information() would return
  "No diagnostic information available." if it is not std::exception
  or boost::exception.

• Clean up any mentions of Boost from README.md and CMakeLists.txt.

[Sjors]

I'm a fan of obliterating boost. @ryanofsky does this also let you drop native_boost from bitcoin/bitcoin#16367? Since libmultiprocess is an optional dependency anyway, it's probably acceptable to depend on C++17.

[ryanofsky]

Yep, it'd allow dropping native_boost

[vasild]

I was not sure about the switch from C++14 to 17, but thought "Why not, given its already different than bitcoin core's C++11?". Worst case - this PR waits until bitcoin core switches to C++17.

[ryanofsky]

This makes sense. Maybe C++14 would be compatible with more systems than 17, but I enabled 14 here before the bitcoin discussion turned to skipping 14 and going right to 17.

If 17 won't work for some reason, Jeremy Rubin recently linked to two small optional<> replacements in IRC which could also be used here

• https://github.com/martinmoene/optional-bare/blob/master/include/nonstd/optional.hpp
• https://github.com/tcbrindle/cpp17_headers/blob/master/include/stx/optional.hpp

[ryanofsky]

I ran into some issues trying to use this with bitcoin. The biggest problem is that boost::optional is used in many bitcoin interfaces (example findFork), so even if libmultiprocess is going to stop using boost::optional internally, something has to be done to support it externally. I think I can do it in bitcoin/bitcoin#10102, but it will require some more work.

I also needed two tweaks to fix some simpler errors:

diff --git a/include/mp/proxy-io.h b/include/mp/proxy-io.h
index 8cfb87a..ec5d00a 100644
--- a/include/mp/proxy-io.h
+++ b/include/mp/proxy-io.h
@@ -12,6 +12,7 @@
 
 #include <capnp/rpc-twoparty.h>
 
+#include <assert.h>
 #include <functional>
 #include <map>
 #include <memory>
diff --git a/pkgconfig/libmultiprocess.pc.in b/pkgconfig/libmultiprocess.pc.in
index 506e356..d8bda42 100644
--- a/pkgconfig/libmultiprocess.pc.in
+++ b/pkgconfig/libmultiprocess.pc.in
@@ -9,4 +9,4 @@ Description: Multiprocess IPC library
 Version: 0.0
 
 Libs: -L${libdir} -lmultiprocess -L${capnp_prefix}/lib -lcapnp-rpc -lcapnp -lkj-async -lkj -pthread -lpthread
-Cflags: -std=c++14 -I${includedir} -I${capnp_prefix}/include -pthread
+Cflags: -std=c++17 -I${includedir} -I${capnp_prefix}/include -pthread

[Sjors]

@ryanofsky alternatively you could add c++17 support to Bitcoin Core's Optional wrapper?

[vasild]

I tried some things that did not work, need to study this more but I feel like it should be possible to resolve the issue solely on the libmultiprocess side and still not have any traces of boost in it.

In the meantime I added the fix for libmultiprocess.pc.in, split in two commits (C++17 switch + boost removal) and rebased.

[ryanofsky]

Thanks! I should be able to merge this soon. I just need to add ReadField/BuildField overloads for boost::optional in bitcoin/bitcoin#10102 before merging this

[ryanofsky]

I was able to get ReadField/BuildField overloads working, so "Obliterate Boost" commit 10b5c69 no longer causes compile errors.

But it turns out this problem was masking another problem: some really confusing link errors caused by the earlier "Switch from C++14 to C++17" commit f4112b7. This problem is also fixable, but will require doing more work on the bitcoin/bitcoin#16367 build system, adding a new library for c++17 files instead of using the existing libbitcoin_common.a library.

The problem happens when linking executables such as bitcoin-tx or bitcoin_bench that link against libbitcoin_common.a but don't link against libmultiprocess.a. It turns out that compiling with c++17 causes some standard library symbols to be weakly defined instead of undefined. For example the string::size() symbol is compiled as

0000000000000000 W _ZNKSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE4sizeEv

with c++17, and

                 U _ZNKSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEE4sizeEv

with c++11.

This makes it impossible to include c++11 and unused c++17 object files in the same library unless all the dependencies of the c++17 objects are fully satisfied, because even when the c++17 objects are unused and unneeded, the linker will think they are needed because of the weak definitions they contain. So the result is a flood of link errors with f4112b7 when the linker fails to fully link all the unneeded c++17 object files.

I should be able to fix this in bitcoin/bitcoin#16367 by splitting up the libbitcoin_common.a library. Just wanted to note the issue here.

[ryanofsky]

@ryanofsky alternatively you could add c++17 support to Bitcoin Core's Optional wrapper?

Yes, another approach could extend the --enable-multiprocess option make the Optional wrapper use std::optional instead of boost::optional. But this would require extending scope of --enable-multiprocess in awkward ways: either making it compile the whole project with c++17 or building two copies of each library in c++17 and c++11 modes for multiprocess and non-multiprocess binaries, which would be slow and also awkward to do with autotools.

[Sjors]

It's also not ideal for deterministic building if --enable-multiprocess changes the bitcoind binary in any way.

[ryanofsky]

I should be able to fix this in bitcoin/bitcoin#16367 by splitting up the libbitcoin_common.a library. Just wanted to note the issue here.

Forgot to write an update, but this was the approach I took to resolve the problem. All the c++17 objects are now isolated into a libbitcoin_ipc.a library instead of being part of libbitcoin_common.a, so they don't interfere when linking the c++11 objects in libbitcoin_common.a