build: use `BOOST_MULTI_INDEX_ENABLE_SAFE

fanquake commented at 12:20 pm on February 20, 2022: member

Use of this macro enables precondition checks for iterators and functions of the library. It’s use is recommended in debug builds. See https://www.boost.org/doc/libs/1_78_0/libs/multi_index/doc/tutorial/debug.html for more info.

There is also a BOOST_MULTI_INDEX_ENABLE_INVARIANT_CHECKING macro:

When this mode is in effect, all public functions of Boost.MultiIndex will perform post-execution tests aimed at ensuring that the basic internal invariants of the data structures managed are preserved.

fanquake added the label Build system on Feb 20, 2022

theStack commented at 11:51 am on March 3, 2022: member

Concept ACK

theStack approved

theStack commented at 10:14 am on March 4, 2022: member

Tested ACK fe565fbece6ac2bf3de6410de777dbb09885f574

FWIW, executing the unit tests for debug builds doesn’t take significantly longer with this patch on my machine:

0--with-debug build on master branch:
1$ time ./src/test/test_bitcoin
215m05.76s real    12m11.82s user     6m03.60s system
3
4--with-debug build on PR branch:
5$ time ./src/test/test_bitcoin
615m11.04s real    12m05.47s user     6m43.91s system

(I didn’t execute more than once each, i.e. the results have to be taken with a grain of salt)

fanquake requested review from mzumsande on Mar 5, 2022

in configure.ac:1441 in fe565fbece outdated

1422@@ -1423,6 +1423,10 @@ if test "$use_boost" = "yes"; then
1423   if test "$suppress_external_warnings" != "no"; then
1424     BOOST_CPPFLAGS=SUPPRESS_WARNINGS($BOOST_CPPFLAGS)
1425   fi
1426+
1427+  if test "$enable_debug" = "yes"; then
1428+    AX_CHECK_PREPROC_FLAG([-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE], [DEBUG_CPPFLAGS="$DEBUG_CPPFLAGS -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"], [], [$CXXFLAG_WERROR])

luke-jr commented at 9:38 pm on March 5, 2022:

AX_CHECK_PREPROC_FLAG doesn’t really do anything special for -D options. Maybe just add it to DEBUG_CPPFLAGS unconditionally?

fanquake commented at 10:20 am on March 17, 2022:

We are currently using AX_CHECK_PREPROC_FLAG for all other -D options we pass to our DEBUG_CPPFLAGS, so I think I’ll leave this as-is.

fanquake commented at 11:10 am on May 17, 2022:

Now added unconditionally given we’ve got other unconditional flags.

JeremyRubin commented at 0:24 am on March 6, 2022: contributor

is there a failing example test case that we can run to verify it is enabled in debug mode?

Or should these invariants be unreachable for extenral users?

mzumsande commented at 1:11 am on March 6, 2022: member

Partial Tested ACK fe565fbece6ac2bf3de6410de777dbb09885f574 (can’t say anything about the configure.ac flag details, which I don’t know anything about )

is there a failing example test case that we can run to verify it is enabled in debug mode?

I tried to dereference an iterator to an entry of an empty mempool:

0BOOST_AUTO_TEST_CASE(mempool_iterator_ub)
1{
2    CTxMemPool pool;
3    LOCK2(cs_main, pool.cs);
4    typename CTxMemPool::indexed_transaction_set::index<descendant_score>::type::iterator it = pool.mapTx.get<descendant_score>().begin();
5    std::string txId{it->GetTx().GetHash().ToString()};
6}

This fails for me on this branch with

0(...)Assertion `safe_mode::check_dereferenceable_iterator(*this)' failed.

and passes on master. Triggering other codes from safe_mode::error_code (see link in OP) should be possible too.

FWIW, executing the unit tests for debug builds doesn’t take significantly longer with this patch on my machine:

The benchmark ComplexMemPool is slower by a factor of roughly 3 compared to a --enable-debug build on master. PR:

ns/op	op/s	err%	total	benchmark
3,003,964,479.00	0.33	0.6%	33.01	`ComplexMemPool`

Master:

ns/op	op/s	err%	total	benchmark
1,182,466,622.00	0.85	1.6%	13.03	`ComplexMemPool`

mzumsande commented at 1:25 am on March 6, 2022: member

Would it make sense to enable this also for fuzzing, or do the existing sanitizers already cover these error conditions anyway?

fanquake commented at 10:21 am on March 17, 2022: member

Would it make sense to enable this also for fuzzing, or do the existing sanitizers already cover these error conditions anyway? @MarcoFalke any thoughts?

MarcoFalke commented at 1:23 pm on March 17, 2022: member

I think google oss-fuzz has --enable-debug turned on? Also, there might be a CI task in the qa-assets repo that has it turned on.

JeremyRubin commented at 4:32 pm on March 17, 2022: contributor

you should turn it on for fuzzing, i can think of some implementations that might not be caught in existing sanitizers (e.g., imagine as an optimization deleting a key puts the cleared object in a valid but not initialized ‘orphanage’ for a future allocation to use – the iter might be invalid but not a memory fault to access afterwards).

i dont think the code actually does this, but the internal safe mode would catch these things.

MarcoFalke commented at 4:45 pm on March 17, 2022: member

Oh, I was wrong. OSS-Fuzz does not run with --enable-debug. https://github.com/google/oss-fuzz/blob/aa83381257bbf55d71c6a4b0f56f805409776047/projects/bitcoin-core/build.sh#L57

It only runs with DEBUG=1, but this is disabled due to bugs (:smiling_face_with_tear: ): https://github.com/google/oss-fuzz/blob/aa83381257bbf55d71c6a4b0f56f805409776047/projects/bitcoin-core/build.sh#L33

I am running --enable-debug somewhere else: https://github.com/MarcoFalke/btc_nightly/blob/0b191f4cb53134217f992908a0f5962dcc05b6f7/00_setup_env_native_fuzz_enable_debug.sh#L17

Though that also fails currently due to a boost bug (:smiling_face_with_tear: ): https://cirrus-ci.com/task/6626378031300608?logs=fuzz_run#L947

If you enable it for fuzzing in this repo, the mempool test might take three times as long. (https://cirrus-ci.com/task/4976712239808512?logs=ci#L4448) From 30 minutes to 90 minutes. Not sure if this will push us over the 120 minute timeout.

fanquake force-pushed on Apr 4, 2022

fanquake commented at 10:10 am on April 4, 2022: member

Rebased this given #24558 has been merged.

MarcoFalke added the label DrahtBot Guix build requested on Apr 21, 2022

DrahtBot commented at 10:01 am on April 23, 2022: member

Guix builds

File	commit 346e780442f91fc155dcc9c44eedf23ac0bb15a7(master)	commit 06f2e412916e3e8b65c2cd68b63bcc60244d55ef(master and this pull)
SHA256SUMS.part	`791ceba7511820bd...`	`f61dce7e030666d8...`
*-aarch64-linux-gnu-debug.tar.gz	`c4394ec90823ac39...`	`1bb177acb40472ce...`
*-aarch64-linux-gnu.tar.gz	`49e745c299086c99...`	`9979bad174e583e9...`
*-arm-linux-gnueabihf-debug.tar.gz	`35633cce17d629eb...`	`2e84605bfd931a68...`
*-arm-linux-gnueabihf.tar.gz	`0761e21f2e62ed95...`	`0039844c8bb32acf...`
*-arm64-apple-darwin-unsigned.dmg	`5daa39fc518c86ae...`	`07fd8d65706ea357...`
*-arm64-apple-darwin-unsigned.tar.gz	`cb9d396cfa8666db...`	`f1e899eae70fcc59...`
*-arm64-apple-darwin.tar.gz	`60f8f27f4b93a533...`	`67863e0d7d63c9bf...`
*-powerpc64-linux-gnu-debug.tar.gz	`ecc3060a762707f1...`	`5c6aea34ce95313f...`
*-powerpc64-linux-gnu.tar.gz	`18b2b694385817e4...`	`0597915aad4ff786...`
*-powerpc64le-linux-gnu-debug.tar.gz	`e65825425df649a4...`	`3f7565e89f0dc54f...`
*-powerpc64le-linux-gnu.tar.gz	`cf5d5221c17adf07...`	`66a624de33e97a69...`
*-riscv64-linux-gnu-debug.tar.gz	`e724ba323541cfae...`	`f922cb7fa75b2e16...`
*-riscv64-linux-gnu.tar.gz	`f9454b4a737f3969...`	`dcf1d060fd016f66...`
*-win64-debug.zip	`4c330fdd8159b833...`	`fe5aa3d57bff3fff...`
*-win64-setup-unsigned.exe	`57ae88cdfecd4c3f...`	`c26b38a47bb6b9c4...`
*-win64-unsigned.tar.gz	`cf8abf09f23d3db1...`	`cfbb14bb0c705629...`
*-win64.zip	`0eb863575ba5da10...`	`213c122146d867c3...`
*-x86_64-apple-darwin-unsigned.dmg	`27f2d9448832e6e8...`	`9b3cc97fdaca0b67...`
*-x86_64-apple-darwin-unsigned.tar.gz	`ff3b4f667dcfdd1d...`	`d66e1a78bcee83fe...`
*-x86_64-apple-darwin.tar.gz	`8eb9c9fc8611b7c7...`	`0e3e8316bf025f1d...`
*-x86_64-linux-gnu-debug.tar.gz	`040f6196974aa498...`	`b9c0a8f3bfbbfda7...`
*-x86_64-linux-gnu.tar.gz	`db199b6a60087d64...`	`7bbac0b51850fd6a...`
*.tar.gz	`d3b56c48d0c9a3fd...`	`c93ea571c752a9a4...`
guix_build.log	`91802c9a48e3e323...`	`e3ad46266df9ee16...`
guix_build.log.diff		`facdf995862eb025...`

DrahtBot removed the label DrahtBot Guix build requested on Apr 23, 2022

fanquake force-pushed on May 17, 2022

fanquake commented at 11:09 am on May 17, 2022: member

Rebased and simplified.

in configure.ac:1458 in d581ee0b40 outdated

1454@@ -1455,6 +1455,10 @@ if test "$use_boost" = "yes"; then
1455   dnl we don't use multi_index serialization
1456   BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_DISABLE_SERIALIZATION"
1457 
1458+  if test "$enable_debug" = "yes"; then

MarcoFalke commented at 6:04 am on May 19, 2022:

Could also enable on –enable-fuzz ?

fanquake commented at 8:44 am on May 19, 2022:

Added.

build: use BOOST_MULTI_INDEX_ENABLE_SAFE_MODE when debugging

Use of this macro enables precondition checks for iterators and
functions of the library. It's use is recommended in debug builds.

See:
https://www.boost.org/doc/libs/1_78_0/libs/multi_index/doc/tutorial/debug.html.

06e18e0b53

fanquake force-pushed on May 19, 2022

MarcoFalke commented at 9:29 am on May 20, 2022: member

Looks like this doesn’t affect the fuzz runtime too much, so should be fine.

fanquake requested review from laanwj on Jun 1, 2022

laanwj commented at 3:01 pm on June 8, 2022: member

Concept and code review ACK 06e18e0b53ed34933ecd7f3976a508be72f687aa

MarcoFalke merged this on Jun 8, 2022

MarcoFalke closed this on Jun 8, 2022

fanquake deleted the branch on Jun 8, 2022

sidhujag referenced this in commit e79db12076 on Jun 13, 2022

DrahtBot locked this on Jun 8, 2023

build: use BOOST_MULTI_INDEX_ENABLE_SAFE_MODE when debugging #24395

Guix builds

build: use `BOOST_MULTI_INDEX_ENABLE_SAFE_MODE` when debugging #24395