build: disable boost multi index safe mode in debug mode #27724

willcl-ark · 2023-05-23T07:56:47Z

Disable boost multi index safe mode by default when configuring with
--enable-debug.

This option can cause transactions to take a long time to be accepted
into the mempool under certain conditions; iterator destruction takes
O(n) time vs O(1) as they are stored in a singly linked list. See
27586 and the boost docs for more information.

Re-enable it on the CI builds which previously had it enabled.

Re-enable it on the msan fuzz task so that we have fuzz tasks testing
with it enabled and disabled in this repo.

DrahtBot · 2023-05-23T07:56:50Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fanquake
Concept ACK	MarcoFalke, hebasto

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27737 (ci: compile Clang and compiler-rt in msan jobs by fanquake)
#25797 (build: Add CMake-based build system by hebasto)

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.

hebasto · 2023-05-23T07:59:10Z

Somewhat related: #27353.

willcl-ark · 2023-05-23T08:00:34Z

The approach taken here was influenced by discussion in #24709 (comment)

I've been unable to instigate the congested behaviour on two nodes overnight, one with default mempool and one with 500MB mempool, but hopefully will manage to do so shortly and will reply here if so.

In any case, it seems overkill to enable these safe mode checks in debug builds (which are often tested on mainnet), and a more sensible approach to limit to CI tasks where performance is less critical.

fanquake · 2023-05-23T08:01:27Z

Concept NACK - I don't think adding configure flags for specific Boost defines is a good approach. Will follow up in the other threads.

willcl-ark · 2023-05-23T08:16:15Z

Concept NACK - I don't think adding configure flags for specific Boost defines is a good approach. Will follow up in the other threads.

OK. Looking forward to your thoughts, as I don't know how the same effect could be achieved in any neater or more maintainable way.

One alternative could be to manually append to the CI CPP flags, if that's all we wanted.

maflcko · 2023-05-23T08:41:57Z

One alternative could be to manually append to the CI CPP flags, if that's all we wanted.

Or just a configure setting to append CPP flags to the existing ones, instead of replacing the default ones?

Concept ACK

fanquake · 2023-05-23T08:45:28Z

Or just a configure setting to append CPP flags to the existing ones,

There's no need for a new setting. Just use CPPFLAGS="-DWHATEVER". This has worked since #24391.

maflcko · 2023-05-23T09:15:41Z

configure.ac

@@ -1491,7 +1491,7 @@ if test "$use_boost" = "yes"; then
  AX_CHECK_PREPROC_FLAG([-DBOOST_NO_CXX98_FUNCTION_BASE], [BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_NO_CXX98_FUNCTION_BASE"], [], [$CXXFLAG_WERROR],
                        [AC_LANG_PROGRAM([[#include <boost/config.hpp>]])])

-  if test "$enable_debug" = "yes" || test "$enable_fuzz" = "yes"; then
+  if test "$enable_fuzz" = "yes"; then
    BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"


I wonder if this should be moved to the debug fuzz task to avoid only fuzzing a debug version of boost in all fuzz configs?

I like that. If anything it makes more sense to me to fuzz the non debug version...

ideally both are fuzzed :)

Right.

Do you think it's best to add to 00_setup_env_native_fuzz.sh ?

Oh, I meant 00_setup_env_native_fuzz_debug.sh, but I see that this is maintained elsewhere. Maybe it can be added to just fuzz_msan, or just no fuzz task at all in this repo?

maflcko

lgtm, but the description needs to be adjusted?

maflcko · 2023-05-23T09:18:10Z

configure.ac

@@ -1491,7 +1491,7 @@ if test "$use_boost" = "yes"; then
  AX_CHECK_PREPROC_FLAG([-DBOOST_NO_CXX98_FUNCTION_BASE], [BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_NO_CXX98_FUNCTION_BASE"], [], [$CXXFLAG_WERROR],
                        [AC_LANG_PROGRAM([[#include <boost/config.hpp>]])])

-  if test "$enable_debug" = "yes" || test "$enable_fuzz" = "yes"; then
+  if test "$enable_fuzz" = "yes"; then
    BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"


ideally both are fuzzed :)

maflcko

can squash to avoid touching the same line twice?

Disable boost multi index safe mode by default when configuring with --enable-debug. This option can cause transactions to take a long time to be accepted into the mempool under certain conditions; iterator destruction takes O(n) time vs O(1) as they are stored in a singly linked list. See 27586 for more information. Re-enable it on the CI builds which previously had it enabled. Re-enable it on the msan fuzz target so that we have fuzz tasks testing with it enabeld and disabled in this repo.

willcl-ark · 2023-05-23T12:45:26Z

can squash to avoid touching the same line twice?

Now squashed :)

hebasto

~~ACK 59c8944~~

UPD. See #27724 (comment)

hebasto · 2023-05-23T12:57:57Z

ci/test/00_setup_env_native_fuzz_with_msan.sh

@@ -17,7 +17,7 @@ export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev cmake"
 # BDB generates false-positives and will be removed in future
 export DEP_OPTS="NO_BDB=1 NO_QT=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory --disable-hardening --with-asm=no CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
+export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory --disable-hardening --with-asm=no CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"


nit: Could split the long line?

Sure, will do if I touch again :)

I did think about it at the time, but some CI tasks seem to be split on ~100 chars, and some just have long lines. So I just left this one as "long" (and made it longer).

hebasto · 2023-05-23T14:10:59Z

Retracting my ACK, as FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_msan.sh" ./ci/test_run_all.sh fails locally for me.

maflcko · 2023-05-23T14:37:51Z

Retracting my ACK, as FILE_ENV="./ci/test/00_setup_env_native_fuzz_with_msan.sh" ./ci/test_run_all.sh fails locally for me.

Seems unrelated, as this is failing on master. I agree it should be fixed, but I don't see why something unrelated should be holding back this pull.

willcl-ark · 2023-05-23T14:45:57Z

Perhaps I can drop the fuzz CPPFLAGS (introduced in this pull) for now, so that nothing is broken after this commit?

diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
index dd694f818c..35a0de8034 100755
--- a/ci/test/00_setup_env_native_fuzz_with_msan.sh
+++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh
@@ -17,7 +17,7 @@ export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev cmake"
 # BDB generates false-positives and will be removed in future
 export DEP_OPTS="NO_BDB=1 NO_QT=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
 export GOAL="install"
-export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory --disable-hardening --with-asm=no CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CPPFLAGS='-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
+export BITCOIN_CONFIG="--enable-fuzz --with-sanitizers=fuzzer,memory --disable-hardening --with-asm=no CC=clang CXX=clang++ CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
 export USE_MEMORY_SANITIZER="true"
 export RUN_UNIT_TESTS="false"
 export RUN_FUNCTIONAL_TESTS="false"

FWIW I do see the same (I guess) locally. At least I see failure during configure:

configure: exit 1
+ false

maflcko · 2023-05-23T14:54:10Z

... so that nothing is broken after this commit?

Again, this is unrelated, because the issue happens in master. See also https://cirrus-ci.com/task/4517884630663168?logs=ci#L4910

fanquake · 2023-05-24T10:24:42Z

Again, this is unrelated, because the issue happens in master. See also https://cirrus-ci.com/task/4517884630663168?logs=ci#L4910

Should be fixed in #27737.

fanquake

ACK 59c8944

Disable boost multi index safe mode by default when configuring with --enable-debug. This option can cause transactions to take a long time to be accepted into the mempool under certain conditions; iterator destruction takes O(n) time vs O(1) as they are stored in a singly linked list. See 27586 for more information. Re-enable it on the CI builds which previously had it enabled. Re-enable it on the msan fuzz target so that we have fuzz tasks testing with it enabeld and disabled in this repo. Github-Pull: bitcoin#27724 Rebased-From: 59c8944

fanquake · 2023-05-29T16:16:32Z

Backported to 25.x in #27775.

…bug mode

9dc5848 build: disable boost multi index safe mode (willcl-ark) Pull request description: Backports #27724 to `25.x`. ACKs for top commit: stickies-v: ACK 9dc5848 Tree-SHA512: 5f156424bbd86caac6ace933b807cd62a21067bcfa7f572d6efeff2622ba0b15401038a3b06fe00d84cf62b5d9b8a9e101650d145683a21fa890c18a9c2bd4b6

This change encompasses: - bitcoin#24558 - bitcoin#27724

DrahtBot added the Build system label May 23, 2023

willcl-ark force-pushed the disable-boost-MI-safe-mode branch from dc606c3 to 26ec68a Compare May 23, 2023 09:02

maflcko reviewed May 23, 2023

View reviewed changes

maflcko approved these changes May 23, 2023

View reviewed changes

willcl-ark force-pushed the disable-boost-MI-safe-mode branch from 26ec68a to 19db479 Compare May 23, 2023 11:50

maflcko approved these changes May 23, 2023

View reviewed changes

willcl-ark force-pushed the disable-boost-MI-safe-mode branch from 19db479 to 59c8944 Compare May 23, 2023 12:45

hebasto approved these changes May 23, 2023

View reviewed changes

This was referenced May 24, 2023

ci: compile Clang and compiler-rt in msan jobs #27737

Merged

build: Add CMake-based build system #25797

Closed

fanquake approved these changes May 29, 2023

View reviewed changes

fanquake merged commit fb4f047 into bitcoin:master May 29, 2023

fanquake mentioned this pull request May 29, 2023

[25.x] build: disable boost multi index safe mode #27775

Merged

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 29, 2023

Merge bitcoin#27724: build: disable boost multi index safe mode in de…

1cdc289

…bug mode

willcl-ark deleted the disable-boost-MI-safe-mode branch May 30, 2023 07:18

hebasto mentioned this pull request Jun 5, 2023

refactor (tidy): Fixes after enable-debug configure #27353

Closed

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 10, 2023

[FIXUP] Boost

91d7327

This change encompasses: - bitcoin#24558 - bitcoin#27724

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 11, 2023

[FIXUP] Boost

92a907f

This change encompasses: - bitcoin#24558 - bitcoin#27724

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 11, 2023

[FIXUP] Boost

77fa55f

This change encompasses: - bitcoin#24558 - bitcoin#27724

hebasto added a commit to hebasto/bitcoin that referenced this pull request Jul 12, 2023

[FIXUP] Boost

38a0b9e

This change encompasses: - bitcoin#24558 - bitcoin#27724

losh11 mentioned this pull request Feb 18, 2024

Vulnerability in 24.0.1 bitcoin upstream: CVE-2023-33297 (ONLY affects the unmerged .24 branches) litecoin-project/litecoin#913

Closed

bitcoin locked and limited conversation to collaborators May 29, 2024

build: disable boost multi index safe mode in debug mode #27724

build: disable boost multi index safe mode in debug mode #27724

Uh oh!

Conversation

willcl-ark commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

hebasto commented May 23, 2023

Uh oh!

willcl-ark commented May 23, 2023

Uh oh!

fanquake commented May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willcl-ark commented May 23, 2023

Uh oh!

maflcko commented May 23, 2023

Uh oh!

fanquake commented May 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented May 23, 2023

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 23, 2023

Uh oh!

maflcko commented May 23, 2023

Uh oh!

willcl-ark commented May 23, 2023

Uh oh!

maflcko commented May 23, 2023

Uh oh!

fanquake commented May 24, 2023

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented May 29, 2023

Uh oh!

Uh oh!

willcl-ark commented May 23, 2023 •

edited

Loading

DrahtBot commented May 23, 2023 •

edited

Loading

fanquake commented May 23, 2023 •

edited

Loading

hebasto left a comment •

edited

Loading