Upgrade to Boost 1.65

[jmjatlanta]

This PR will allow bitshares-fc to be compiled with Boost 1.661.65.

• This relies on boost::stacktrace, which was introduced in Boost 1.65. The CMakeLists.txt file will need to be modified to conditionally include the boost::stacktrace that is in the boost_1.51 directory (if PR Issue 727b - A second option #33 is merged in)

[abitmore]

Perhaps add a version check somewhere if not done already, so if someone doesn't have boost 1.65 with backtrace, it can still compile.

By the way, bitshares/bitshares-core#597 is related. I don't know whether this PR will introduce other issues.

[pmconrad]

Glad to see all the old code go!
Seems to work with latest develop. (Edit: only tested with old boost at that time.)

[pmconrad]

AFAICS boost::hash<fc::sha256> isn't used anywhere, so you should be able to remove the include and the template specialization below.
I'm not particularly happy with it (and std::hash<...>) because the return value is architecture-dependent. I don't think this is used in a consensus-related context though, so not that bad.

[abitmore]

Perhaps off-topic here, perhaps std::hash is used in hashed_unique indexes?

[jmjatlanta]

At first, I actually commented out the sha256 specialization that needed this header, and it compiled and ran fine. With the other changes being fairly easy, this was the change that I questioned most. So instead of commenting it out, I put it back in and added the header. I will do more research to see if I can find anywhere that it is being used.

[pmconrad]

Perhaps off-topic here, perhaps std::hash is used in hashed_unique indexes?

Yes, std::hash is but boost::hash isn't.

[jmjatlanta]

I removed boost::hashfc::sha256 from the file. Everything compiles and runs fine without it. Please take a look at the latest commit.

[pmconrad]

I have built bitshares-core with this version of fc plus the stacktrace PR in the following combinations:

• boost-1.60 + gcc-5 (my usual setup)
• boost-1.63 + gcc-5
• boost-1.63 + gcc-6
• boost-1.66 + gcc-6

In the build with boost-1.66, chain_test produces 6 failures that are not present in the other builds. There is something broken there. The fc testsuite itself is fine with both boost-1.66 and boost-1.60.

[pmconrad]

Strange. witness_node built with 1.66 was able to replay and connect to the live network, cli_wallet also seems to work.
Can anyone else reproduce the failures in chain_test with 1.66?

[jmjatlanta]

Yes, I can reproduce the 6 errors in chain_test. It looks like an exception throws another exception, leading to a terminate. I am researching.

[jmjatlanta]

I'm picking on the failure of operation_tests/call_order_update_test.

As part of the database undo due to line 144 ("attempting to claim all collateral without paying off debt"), the first task of the undo is to get the object id 1.8.1 (which I believe is the call order) from the database. The Boost 1.63 version does that just fine. The boost 1.66 version does not. It can retrieve the index, but asking the index for the object throws the fc::exception (see undo_database.h line 68).

Update1: Commenting out line 144 allows the rest of the test to finish, including another cover() that throws an exception (as it should). So just the cover() on line 144 seems to have an issue. It is also not the first cover() in the test.

Update2: Commenting out line 144 allows ALL the other tests to finish without a problem. So that line is affecting other tests.

[jmjatlanta]

I have narrowed it down to some commits in the multi_index library of boost. Between Boost 1.65 and Boost 1.66, there were some changes that appear innocuous, but are causing problems. It is curious that much of our code is working fine, but adding a call_order_object to the index is not.

I am now trying to determine if it is a boost bug, or simply the manner in which we are using it. The problem still exists with the 1.67 tag of boost::multi_index, so if it is their bug, they haven't seen it yet.

Update1: I think I found the issue: https://www.boost.org/doc/libs/1_67_0/libs/multi_index/doc/release_notes.html#boost_1_66

We throw due to the call order not being legitimate, and boost rolls back the change. Then we attempt to "undo" the database, which we think contains the call_order_object (among others). But that object was never added, and our lookup fails.

I am now looking for solutions. Possibilities are:

1. Don't throw... find another way to communicate the problem
2. Don't roll back that operation, as it was not added... implementation seems harder, but still evaluating.
3. Catch the problem earlier in the process, so the roll back does not contain the (non-existent) operation

[jmjatlanta]

Boost 1.66 introduces a breaking change (see boost link above) that rolls back index modifications on exceptions. This duplicates our undo process, and causes problems.

A separate issue was created at bitshares/bitshares-core#852 for tracking.

[pmconrad]

Tested with 1.65.1, no more errors

[abitmore]

Someone please help check whether this PR will compile on Ubuntu 16.04 LTS with shipped boost 1.58 libs? Thanks.

//Update:
Best also check boost 1.57 as it's mentioned everywhere.

[jmjatlanta]

Testing with bitshares-core (develop branch), Ubuntu 16.04 LTS and Boost 1.58...

bitshares-fc tests:
tests/blinding_test reporting 36 failures

bitshares-core tests:
app_test reporting 1 failure
chain_bench reporting 1 failure

I will investigate to compare these results with Boost 1.65.

[pmconrad]

I have to admit I ignored these. app_test and chain_bench are unmaintained, blinding_tests tests an unused feature. AFAIK all of these have been broken for quite some time.

[jmjatlanta]

Update: The versions of Boost on my system were bleeding into each other, so I'm invalidating all previous tests. I have now isolated the boost versions, and am starting again with Boost 1.57.

Test	Boost 1.57	Boost 1.58.0	Boost 1.63	Boost 1.65.1	Boost 1.66
fc::blinding_test	36 failures	36 failures	36 failures	36 failures	36 failures
core:chain_bench	1 failure	1 failure	1 failure	1 failure	1 failure
core:chain_test	0 failures	0 failures	0 failures	0 failures	6 failures

All other tests passed.

[abitmore]

@jmjatlanta I'd recommend not only update old comments in issues/PRs, since no new email notification would be sent out when updating a comment, participants would miss important updates. If it's better to update a comment for better context, and if the update is important, please add a new comment so others will know there is a update.

[abitmore]

For this PR, I think it's OK to merge it.