refactor: Make bitcoin-util grind_task tsan friendly #26120

pull maflcko wants to merge 1 commits into bitcoin:master from maflcko:2209-tsan-less-sad-💥 changing 1 files +7 −5
  1. maflcko commented at 3:00 PM on September 19, 2022: member

    While there is no issue with the current code, libtsan-12.2.1 on my machine does not seem to like it. This is understandable, because the nonce isn't protected by a mutex that the sanitizer can see (only by an atomic, which achieves the same).

    Fix this by guarding the nonce by the existing atomic bool, which tsan seems to understand.

  2. maflcko added the label Utils/log/libs on Sep 19, 2022
  3. maflcko commented at 3:19 PM on September 19, 2022: member

    To test on a fresh install of Ubuntu Kinetic:

    Install: export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git ./bitcoin-core && cd bitcoin-core && apt install build-essential libtool autotools-dev automake pkg-config bsdmainutils python3-zmq libevent-dev libboost-dev libsqlite3-dev -y && ./autogen.sh && ./configure --with-sanitizers=thread && make -j $(nproc)

    On master:

    $  ./test/functional/tool_signet_miner.py
2022-09-19T14:59:51.684000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_d8sczivn
==================
WARNING: ThreadSanitizer: data race (pid=19721)
  Write of size 4 at 0x7ffc7fce52ec by thread T1:
    [#0](/bitcoin-bitcoin/0/) grind_task src/bitcoin-util.cpp:102 (bitcoin-util+0x1ae8c)
    [#1](/bitcoin-bitcoin/1/) void std::__invoke_impl<void, void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >(std::__invoke_other, void (*&&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&&, std::reference_wrapper<CBlockHeader>&&, int&&, int&&, std::reference_wrapper<std::atomic<bool> >&&) /usr/include/c++/12/bits/invoke.h:61 (bitcoin-util+0x1bb69)
    [#2](/bitcoin-bitcoin/2/) std::__invoke_result<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >::type std::__invoke<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >(void (*&&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&&, std::reference_wrapper<CBlockHeader>&&, int&&, int&&, std::reference_wrapper<std::atomic<bool> >&&) /usr/include/c++/12/bits/invoke.h:96 (bitcoin-util+0x1bb69)
    [#3](/bitcoin-bitcoin/3/) void std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul, 5ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul, 5ul>) /usr/include/c++/12/bits/std_thread.h:252 (bitcoin-util+0x1bb69)
    [#4](/bitcoin-bitcoin/4/) std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > >::operator()() /usr/include/c++/12/bits/std_thread.h:259 (bitcoin-util+0x1bb69)
    [#5](/bitcoin-bitcoin/5/) std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210 (bitcoin-util+0x1bb69)
    [#6](/bitcoin-bitcoin/6/) <null> <null> (libstdc++.so.6+0xdc3a2)
  4. maflcko force-pushed on Sep 19, 2022
  5. DrahtBot commented at 3:26 AM on September 23, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ajtowns, hebasto

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  6. maflcko commented at 12:43 PM on September 29, 2022: member

    CC @ajtowns @kallewoof

  7. in src/bitcoin-util.cpp:127 in fa0f81937d outdated 
     124 |  
 125 |      std::vector<std::thread> threads;
 126 |      int n_tasks = std::max(1u, std::thread::hardware_concurrency());
 127 |      for (int i = 0; i < n_tasks; ++i) {
 128 | -        threads.emplace_back( grind_task, nBits, std::ref(header), i, n_tasks, std::ref(found) );
 129 | +        threads.emplace_back(grind_task, nBits, header, i, n_tasks, std::ref(found));
    ajtowns commented at 5:46 PM on September 29, 2022:

    Maybe just pass in both a std::atomic<bool>& found and a std::atomic<uint32_t>& proposed_nonce with the threads doing:

    if (hash <= target) { found = true; proposed_nonce = header.nNonce; return; }

    and the parent doing

    if (found) { header.nNonce = proposed_nonce }
else { error; }
    ajtowns commented at 6:04 PM on September 29, 2022:

    (gcc seems to treat optional<atomic<int>> and atomic<bool> + atomic<int> pretty similarly; clang seems to need to do calls to atomic_load/atomic_store with the optional which seems lame to me. mostly the code seems easier to read without the .load() and .has_value() stuff. But only very weak preference)

    maflcko commented at 3:26 PM on December 8, 2022:

    Huh? Are you sure this will work. What if the thread is put to sleep after found=true and before setting the nonce. Then, the parent will succeed with if(found), but fail to read the nonce.

    maflcko commented at 3:27 PM on December 8, 2022:

    Oh, I guess it requires the threads to join, which was the requirement before as well.

    maflcko commented at 3:50 PM on December 8, 2022:

    Thanks, done

  8. ajtowns commented at 5:46 PM on September 29, 2022: contributor

    Concept ACK

  9. maflcko force-pushed on Dec 8, 2022
  10. maflcko removed the label Utils/log/libs on Dec 8, 2022
  11. DrahtBot added the label Refactoring on Dec 8, 2022
  12. maflcko requested review from ajtowns on Dec 15, 2022
  13. ajtowns commented at 10:31 PM on December 15, 2022: contributor

    Commit message says "Also, fix l_atomic.m4 bug." but that doesn't seem to be touched?

  14. maflcko force-pushed on Dec 16, 2022
  15. Make bitcoin-util grind_task tsan friendly
    This does not change behavior of the bitcoin-util binary.
    fafcc94398
  16. maflcko force-pushed on Dec 16, 2022
  17. ajtowns approved
  18. ajtowns commented at 5:59 PM on December 16, 2022: contributor

    ACK fafcc9439838b3f084fc054b91bca4b50ee62df5

  19. hebasto commented at 6:29 PM on December 16, 2022: member

    Concept ACK.

  20. hebasto commented at 7:04 PM on December 16, 2022: member

    To test on a fresh install of Ubuntu Kinetic:

    I've managed to reproduce the bug on Ubuntu 22.04 when the build is configured as follows:

    $ ./configure --with-sanitizers=thread CC=gcc-12 CXX=g++-12

    $ ./test/functional/tool_signet_miner.py
2022-12-16T19:01:14.510000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_owlx389u
==================
WARNING: ThreadSanitizer: data race (pid=7604)
  Write of size 4 at 0x7ffea220360c by thread T2:
    [#0](/bitcoin-bitcoin/0/) grind_task src/bitcoin-util.cpp:102 (bitcoin-util+0x15dfc)
    [#1](/bitcoin-bitcoin/1/) void std::__invoke_impl<void, void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >(std::__invoke_other, void (*&&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&&, std::reference_wrapper<CBlockHeader>&&, int&&, int&&, std::reference_wrapper<std::atomic<bool> >&&) /usr/include/c++/12/bits/invoke.h:61 (bitcoin-util+0x16ad9)
    [#2](/bitcoin-bitcoin/2/) std::__invoke_result<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >::type std::__invoke<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > >(void (*&&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&&, std::reference_wrapper<CBlockHeader>&&, int&&, int&&, std::reference_wrapper<std::atomic<bool> >&&) /usr/include/c++/12/bits/invoke.h:96 (bitcoin-util+0x16ad9)
    [#3](/bitcoin-bitcoin/3/) void std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > >::_M_invoke<0ul, 1ul, 2ul, 3ul, 4ul, 5ul>(std::_Index_tuple<0ul, 1ul, 2ul, 3ul, 4ul, 5ul>) /usr/include/c++/12/bits/std_thread.h:252 (bitcoin-util+0x16ad9)
    [#4](/bitcoin-bitcoin/4/) std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > >::operator()() /usr/include/c++/12/bits/std_thread.h:259 (bitcoin-util+0x16ad9)
    [#5](/bitcoin-bitcoin/5/) std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (*)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int, std::reference_wrapper<CBlockHeader>, int, int, std::reference_wrapper<std::atomic<bool> > > > >::_M_run() /usr/include/c++/12/bits/std_thread.h:210 (bitcoin-util+0x16ad9)
    [#6](/bitcoin-bitcoin/6/) <null> <null> (libstdc++.so.6+0xdc2b2)

  Previous read of size 8 at 0x7ffea2203608 by thread T16:
    [failed to restore the stack]

  Location is stack of main thread.

  Location is global '<null>' at 0x000000000000 ([stack]+0x1f608)

  Thread T2 (tid=7607, running) created by main thread at:
    [#0](/bitcoin-bitcoin/0/) pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x63a69)
    [#1](/bitcoin-bitcoin/1/) std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xdc388)
    [#2](/bitcoin-bitcoin/2/) std::thread& std::vector<std::thread, std::allocator<std::thread> >::emplace_back<void (&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&, std::reference_wrapper<CBlockHeader>, int&, int&, std::reference_wrapper<std::atomic<bool> > >(void (&)(unsigned int, CBlockHeader&, unsigned int, unsigned int, std::atomic<bool>&), unsigned int&, std::reference_wrapper<CBlockHeader>&&, int&, int&, std::reference_wrapper<std::atomic<bool> >&&) /usr/include/c++/12/bits/vector.tcc:123 (bitcoin-util+0x13538)
    [#3](/bitcoin-bitcoin/3/) Grind src/bitcoin-util.cpp:130 (bitcoin-util+0x13538)
    [#4](/bitcoin-bitcoin/4/) main src/bitcoin-util.cpp:174 (bitcoin-util+0x13538)

  Thread T16 (tid=7621, running) created by main thread at:
    [#0](/bitcoin-bitcoin/0/) pthread_create ../../../../src/libsanitizer/tsan/tsan_interceptors_posix.cpp:1001 (libtsan.so.2+0x63a69)
    [#1](/bitcoin-bitcoin/1/) std::thread::_M_start_thread(std::unique_ptr<std::thread::_State, std::default_delete<std::thread::_State> >, void (*)()) <null> (libstdc++.so.6+0xdc388)
    [#2](/bitcoin-bitcoin/2/) <null> <null> (libc.so.6+0x29d8f)

SUMMARY: ThreadSanitizer: data race src/bitcoin-util.cpp:102 in grind_task
==================
ThreadSanitizer: reported 1 warnings
Traceback (most recent call last):
  File "/home/hebasto/git/bitcoin/contrib/signet/miner", line 545, in <module>
    main()
  File "/home/hebasto/git/bitcoin/contrib/signet/miner", line 539, in main
    return args.fn(args)
  File "/home/hebasto/git/bitcoin/contrib/signet/miner", line 418, in do_generate
    block = finish_block(block, signet_solution, args.grind_cmd)
  File "/home/hebasto/git/bitcoin/contrib/signet/miner", line 107, in finish_block
    newheadhex = subprocess.run(cmd, stdout=subprocess.PIPE, input=b"", check=True).stdout.strip()
  File "/usr/lib/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/home/hebasto/git/bitcoin/src/bitcoin-util', 'grind', '00000020f61eee3b63a380a477a063af32b2bbc97c9ff9f01f2c4225e9739881080000002488f4fd845f8c118b3a5c7e0503365f90f7c2400aff74a7a72d7d1f38665a407bc09c63ae77031e00000000']' returned non-zero exit status 66.
2022-12-16T19:01:20.978000Z TestFramework (ERROR): Called Process failed with 'None'
Traceback (most recent call last):
  File "/home/hebasto/git/bitcoin/test/functional/test_framework/test_framework.py", line 134, in main
    self.run_test()
  File "/home/hebasto/git/bitcoin/./test/functional/tool_signet_miner.py", line 51, in run_test
    subprocess.run([
  File "/usr/lib/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/usr/bin/python3', '/home/hebasto/git/bitcoin/contrib/signet/miner', '--cli=/home/hebasto/git/bitcoin/src/bitcoin-cli -datadir=/tmp/bitcoin_func_test_owlx389u/node0', 'generate', '--address=tb1qhnx932grpdh55cd0dg5gh2kcrqqs5n8wf9cs4e', '--grind-cmd=/home/hebasto/git/bitcoin/src/bitcoin-util grind', '--nbits=1d00ffff', '--set-block-time=1671217275']' returned non-zero exit status 1.
2022-12-16T19:01:21.030000Z TestFramework (INFO): Stopping nodes
2022-12-16T19:01:21.134000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_owlx389u
2022-12-16T19:01:21.134000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_owlx389u/test_framework.log
2022-12-16T19:01:21.134000Z TestFramework (ERROR): 
2022-12-16T19:01:21.134000Z TestFramework (ERROR): Hint: Call /home/hebasto/git/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_owlx389u' to consolidate all logs
2022-12-16T19:01:21.134000Z TestFramework (ERROR): 
2022-12-16T19:01:21.134000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
2022-12-16T19:01:21.134000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
2022-12-16T19:01:21.134000Z TestFramework (ERROR):
  21. hebasto approved
  22. hebasto commented at 7:17 PM on December 16, 2022: member

    ACK fafcc9439838b3f084fc054b91bca4b50ee62df5, I have reviewed the code and it looks OK, I agree it can be merged. Confirming that initial bug has been fixed.

  23. maflcko merged this on Dec 17, 2022
  24. maflcko closed this on Dec 17, 2022
  25. maflcko deleted the branch on Dec 17, 2022
  26. sidhujag referenced this in commit a0074b6df6 on Dec 17, 2022
  27. bitcoin locked this on Dec 17, 2023
Contributors
maflckoDrahtBotajtownshebasto
Labels
Refactoring
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: 2026-04-22 06:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me