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

pull willcl-ark wants to merge 1 commits into bitcoin:master from willcl-ark:disable-boost-MI-safe-mode changing 4 files +5 −8
  1. willcl-ark commented at 7:56 am on May 23, 2023: member

    Fixes #27586

    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.

  2. DrahtBot commented at 7:56 am on May 23, 2023: contributor

    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.

  3. DrahtBot added the label Build system on May 23, 2023
  4. hebasto commented at 7:59 am on May 23, 2023: member
    Somewhat related: #27353.
  5. willcl-ark commented at 8:00 am on May 23, 2023: member

    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.

  6. fanquake commented at 8:01 am on May 23, 2023: member
    Concept NACK - I don’t think adding configure flags for specific Boost defines is a good approach. Will follow up in the other threads.
  7. willcl-ark commented at 8:16 am on May 23, 2023: member

    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.

  8. maflcko commented at 8:41 am on May 23, 2023: member

    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

  9. fanquake commented at 8:45 am on May 23, 2023: member

    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.

  10. willcl-ark force-pushed on May 23, 2023
  11. in configure.ac:1495 in 26ec68acd1 outdated 
    1490@@ -1491,7 +1491,7 @@ if test "$use_boost" = "yes"; then
1491   AX_CHECK_PREPROC_FLAG([-DBOOST_NO_CXX98_FUNCTION_BASE], [BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_NO_CXX98_FUNCTION_BASE"], [], [$CXXFLAG_WERROR],
1492                         [AC_LANG_PROGRAM([[#include <boost/config.hpp>]])])
1493 
1494-  if test "$enable_debug" = "yes" || test "$enable_fuzz" = "yes"; then
1495+  if test "$enable_fuzz" = "yes"; then
1496     BOOST_CPPFLAGS="$BOOST_CPPFLAGS -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
    maflcko commented at 9:15 am on May 23, 2023:
    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?
    willcl-ark commented at 9:16 am on May 23, 2023:
    I like that. If anything it makes more sense to me to fuzz the non debug version…
    maflcko commented at 9:18 am on May 23, 2023:
    ideally both are fuzzed :)
    willcl-ark commented at 10:57 am on May 23, 2023:

    Right.

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

    maflcko commented at 11:19 am on May 23, 2023:
    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?
  12. maflcko approved
  13. maflcko commented at 9:18 am on May 23, 2023: member
    lgtm, but the description needs to be adjusted?
  14. willcl-ark force-pushed on May 23, 2023
  15. maflcko approved
  16. maflcko commented at 12:15 pm on May 23, 2023: member
    can squash to avoid touching the same line twice?
  17. build: disable boost multi index safe mode 
    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.
    59c8944749
  18. willcl-ark commented at 12:45 pm on May 23, 2023: member

    can squash to avoid touching the same line twice?

    Now squashed :)

  19. willcl-ark force-pushed on May 23, 2023
  20. in ci/test/00_setup_env_native_fuzz_with_msan.sh:20 in 59c8944749 
    16@@ -17,7 +17,7 @@ export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev cmake"
17 # BDB generates false-positives and will be removed in future
18 export DEP_OPTS="NO_BDB=1 NO_QT=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
19 export GOAL="install"
20-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}'"
21+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}'"
    hebasto commented at 12:57 pm on May 23, 2023:
    nit: Could split the long line?
    willcl-ark commented at 1:00 pm on May 23, 2023:

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

  21. hebasto approved
  22. hebasto commented at 12:58 pm on May 23, 2023: member

    ~ACK 59c89447499bd9d6202269879555b8bc37373aa2~

    UPD. See #27724 (comment)

  23. hebasto commented at 2:10 pm on May 23, 2023: member
    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.
  24. maflcko commented at 2:37 pm on May 23, 2023: member

    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.

  25. willcl-ark commented at 2:45 pm on May 23, 2023: member

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

     0diff --git a/ci/test/00_setup_env_native_fuzz_with_msan.sh b/ci/test/00_setup_env_native_fuzz_with_msan.sh
 1index dd694f818c..35a0de8034 100755
 2--- a/ci/test/00_setup_env_native_fuzz_with_msan.sh
 3+++ b/ci/test/00_setup_env_native_fuzz_with_msan.sh
 4@@ -17,7 +17,7 @@ export PACKAGES="clang-16 llvm-16 libclang-rt-16-dev cmake"
 5 # BDB generates false-positives and will be removed in future
 6 export DEP_OPTS="NO_BDB=1 NO_QT=1 CC='clang' CXX='clang++' CFLAGS='${MSAN_FLAGS}' CXXFLAGS='${MSAN_AND_LIBCXX_FLAGS}'"
 7 export GOAL="install"
 8-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}'"
 9+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}'"
10 export USE_MEMORY_SANITIZER="true"
11 export RUN_UNIT_TESTS="false"
12 export RUN_FUNCTIONAL_TESTS="false"

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

    0configure: exit 1
1+ false
  26. maflcko commented at 2:54 pm on May 23, 2023: member

    … 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

  27. fanquake commented at 10:24 am on May 24, 2023: member

    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.

  28. fanquake approved
  29. fanquake commented at 3:53 pm on May 29, 2023: member
    ACK 59c89447499bd9d6202269879555b8bc37373aa2
  30. fanquake merged this on May 29, 2023
  31. fanquake closed this on May 29, 2023
  32. fanquake referenced this in commit 9dc5848492 on May 29, 2023
  33. fanquake commented at 4:16 pm on May 29, 2023: member
    Backported to 25.x in #27775.
  34. sidhujag referenced this in commit 1cdc289776 on May 29, 2023
  35. willcl-ark deleted the branch on May 30, 2023
  36. fanquake referenced this in commit cda3fe2808 on Jun 2, 2023
  37. hebasto referenced this in commit 91d7327225 on Jul 10, 2023
  38. hebasto referenced this in commit 92a907f612 on Jul 11, 2023
  39. hebasto referenced this in commit 77fa55f132 on Jul 11, 2023
  40. hebasto referenced this in commit 38a0b9ee37 on Jul 12, 2023
  41. bitcoin locked this on May 29, 2024


willcl-ark DrahtBot hebasto fanquake maflcko

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-30 00:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me