Fix chain tip data race and corrupt rest response #25077

pull maflcko wants to merge 4 commits into bitcoin:master from maflcko:2205-tip-race-🤠 changing 12 files +68 −35
  1. maflcko commented at 2:25 PM on May 6, 2022: member

    This fixes two issues:

    • A data race in ActiveChain, which returns a reference to the chain (a std::vector), which is not thread safe. See also below traceback.
    • A corrupt rest response, which returns a blockheight and blockhash, which are unrelated to each other and to the result, as the chain might advance between each call without cs_main held.

    The issues are fixed by taking cs_main and holding it for the required time.

    ==================
WARNING: ThreadSanitizer: data race (pid=32335)
  Write of size 8 at 0x7b3c000008f0 by thread T22 (mutexes: write M131626, write M151, write M131553):
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 (bitcoind+0x501239)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__swap_out_circular_buffer(std::__1::__split_buffer<CBlockIndex*, std::__1::allocator<CBlockIndex*>&>&) /usr/lib/llvm-13/bin/../include/c++/v1/vector:977:5 (bitcoind+0x501239)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__append(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:1117:9 (bitcoind+0x501239)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::resize(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:2046:15 (bitcoind+0x4ffe29)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00)
    [#6](/github-metadata-backup-bitcoin-bitcoin/6/) CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e)
    [#7](/github-metadata-backup-bitcoin-bitcoin/7/) CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf)
    [#8](/github-metadata-backup-bitcoin-bitcoin/8/) node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74)
    [#9](/github-metadata-backup-bitcoin-bitcoin/9/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e)
    [#10](/github-metadata-backup-bitcoin-bitcoin/10/) decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e)
    [#11](/github-metadata-backup-bitcoin-bitcoin/11/) void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e)
    [#12](/github-metadata-backup-bitcoin-bitcoin/12/) std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e)
    [#13](/github-metadata-backup-bitcoin-bitcoin/13/) std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e)
    [#14](/github-metadata-backup-bitcoin-bitcoin/14/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f)
    [#15](/github-metadata-backup-bitcoin-bitcoin/15/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f)
    [#16](/github-metadata-backup-bitcoin-bitcoin/16/) util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f)
    [#17](/github-metadata-backup-bitcoin-bitcoin/17/) decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a)
    [#18](/github-metadata-backup-bitcoin-bitcoin/18/) void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a)
    [#19](/github-metadata-backup-bitcoin-bitcoin/19/) void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a)
  Previous read of size 8 at 0x7b3c000008f0 by main thread:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) operator new(unsigned long) <null> (bitcoind+0x132668)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) node::LoadChainstate(bool, ChainstateManager&, CTxMemPool*, bool, Consensus::Params const&, bool, long, long, long, bool, bool, std::__1::function<bool ()>, std::__1::function<void ()>) src/node/chainstate.cpp:31:14 (bitcoind+0x24de07)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Mutex M131626 (0x7b3c00000898) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_lock <null> (bitcoind+0xda898)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::mutex::lock() <null> (libc++.so.1+0x49f35)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x23cd74)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15863e)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15863e)
    [#6](/github-metadata-backup-bitcoin-bitcoin/6/) std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15863e)
    [#7](/github-metadata-backup-bitcoin-bitcoin/7/) std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15863e)
    [#8](/github-metadata-backup-bitcoin-bitcoin/8/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f)
    [#9](/github-metadata-backup-bitcoin-bitcoin/9/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f)
    [#10](/github-metadata-backup-bitcoin-bitcoin/10/) util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f)
    [#11](/github-metadata-backup-bitcoin-bitcoin/11/) decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x157e6a)
    [#12](/github-metadata-backup-bitcoin-bitcoin/12/) void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x157e6a)
    [#13](/github-metadata-backup-bitcoin-bitcoin/13/) void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x157e6a)
  Mutex M151 (0x55aacb8ea030) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_init <null> (bitcoind+0xbed2f)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) __libc_start_main <null> (libc.so.6+0x29eba)
  Mutex M131553 (0x7b4c000042e0) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_init <null> (bitcoind+0xbed2f)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::__unique_if<CTxMemPool>::__unique_single std::__1::make_unique<CTxMemPool, CBlockPolicyEstimator*, int const&>(CBlockPolicyEstimator*&&, int const&) /usr/lib/llvm-13/bin/../include/c++/v1/__memory/unique_ptr.h:728:32 (bitcoind+0x15c81d)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Thread T22 'b-loadblk' (tid=32370, running) created by main thread at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xbd5bd)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:443:10 (bitcoind+0x155e06)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, void>(void (*&&)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (bitcoind+0x155e06)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 in std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&)
==================

    From https://cirrus-ci.com/task/5612886578954240?logs=ci#L4868

  2. DrahtBot added the label Refactoring on May 6, 2022
  3. in src/init.cpp:1639 in fa35585c74 outdated 
    1627 | @@ -1628,7 +1628,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1628 |      // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
1629 |      // No locking, as this happens before any background thread is started.
1630 |      boost::signals2::connection block_notify_genesis_wait_connection;
1631 | -    if (chainman.ActiveChain().Tip() == nullptr) {
1632 | +    if (WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip() == nullptr)) {
    Crypt-iQ commented at 2:52 PM on May 6, 2022:

    Should callers be using ActiveTip instead of ActiveChain().Tip() since I think Tip() itself doesn't require a mutex and could change?

    Crypt-iQ commented at 2:53 PM on May 6, 2022:

    Nevermind, I see the WITH_LOCK. mb

    maflcko commented at 3:49 PM on May 6, 2022:

    Calling Tip() will need the mutex, as it accesses the underlying std:vector. Using a CBlockIndex* may or may not need the mutex, so I think it it fine to request the caller to figure that out.

    Crypt-iQ commented at 5:28 PM on May 9, 2022:

    Should the CChain functions that use vChain have the exclusive lock annotation?

    maflcko commented at 1:37 PM on June 21, 2022:

    I am not sure if we want to add validation mutex lock annotations to classes that are only supposed to be primitive data structures.

    Maybe for now this could make sense to document the status quo. However, I'd like to postpone this to the future.

    Alternatively, we could start thinking about adding a CChain member mutex, similar to the mempool one?

    Crypt-iQ commented at 9:07 PM on June 21, 2022:

    I think adding a member mutex would be a good idea in the future

  4. DrahtBot commented at 7:57 PM on May 6, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #25704 (refactor: Remove almost all validation option globals by MarcoFalke)
    • #25296 (Add DataStream without ser-type and ser-version and use it where possible by MarcoFalke)

    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.

  5. pk-b2 approved
  6. pk-b2 commented at 4:03 AM on May 7, 2022: none

    ACK fa35585c74ca48d622d0b2fb9bbfb1c2148eeb00

  7. DrahtBot cross-referenced this on May 7, 2022 from issue assumeutxo: add init and completion logic by jamesob
  8. luke-jr referenced this in commit dd6e44b560 on May 21, 2022
  9. luke-jr referenced this in commit d300cd95c0 on May 22, 2022
  10. DrahtBot cross-referenced this on May 25, 2022 from issue CChainState -> Chainstate by jamesob
  11. DrahtBot cross-referenced this on May 26, 2022 from issue [kernel 2d/n] Reduce CTxMemPool constructor call sites by dongcarl
  12. DrahtBot cross-referenced this on Jun 7, 2022 from issue [kernel 3a/n] Decouple `CTxMemPool` from `ArgsManager` by dongcarl
  13. DrahtBot cross-referenced this on Jun 8, 2022 from issue Add DataStream without ser-type and ser-version and use it where possible by maflcko
  14. maflcko cross-referenced this on Jun 10, 2022 from issue How to avoid logic races (and recursive mutexes)? by maflcko
  15. ajtowns commented at 3:59 PM on June 13, 2022: contributor

    Approach ACK. I think this needs a more thorough API cleanup somehow, but this improves things in the meantime.

    This is mostly a bug fix; I don't think it should have the refactoring tag?

  16. maflcko removed the label Refactoring on Jun 13, 2022
  17. maflcko added the label Bug on Jun 13, 2022
  18. maflcko commented at 4:01 PM on June 13, 2022: member

    @DrahtBot Bad bot!!

  19. maflcko added the label Validation on Jun 13, 2022
  20. DrahtBot added the label Needs rebase on Jun 16, 2022
  21. maflcko force-pushed on Jun 21, 2022
  22. maflcko force-pushed on Jun 21, 2022
  23. maflcko commented at 1:39 PM on June 21, 2022: member

    Rebased, answered question, and started removing ::cs_main

  24. in src/validation.h:882 in fa35905c0a outdated 
     854 | +     * - Clarify that the method will acquire a mutex that heavily affects
 855 | +     *   overall performance.
 856 | +     * - Force call sites to think how long they need to acquire the mutex to
 857 | +     *   get consistent results.
 858 | +     */
 859 | +    RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; }
    jamesob commented at 2:21 PM on June 21, 2022:

    Nice, this is a good direction I think. :+1:

  25. jamesob commented at 2:22 PM on June 21, 2022: member

    Concept ACK

  26. DrahtBot removed the label Needs rebase on Jun 21, 2022
  27. Crypt-iQ commented at 11:03 PM on June 21, 2022: contributor

    crACK fa35905c0a5d45ab16283536fb98124db71ef897

  28. Riahiamirreza approved
  29. in src/validation.h:955 in fa35905c0a outdated 
     927 | @@ -915,9 +928,9 @@ class ChainstateManager
 928 |  
 929 |      //! The most-work chain.
 930 |      CChainState& ActiveChainstate() const;
 931 | -    CChain& ActiveChain() const { return ActiveChainstate().m_chain; }
 932 | -    int ActiveHeight() const { return ActiveChain().Height(); }
 933 | -    CBlockIndex* ActiveTip() const { return ActiveChain().Tip(); }
 934 | +    CChain& ActiveChain() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChainstate().m_chain; }
 935 | +    int ActiveHeight() const EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) { return ActiveChain().Height(); }
    ajtowns commented at 12:05 PM on July 4, 2022:

    Needs { LOCK(::cs_main); ... } added to bitcoin-chainstate.cpp around the Main program logic starts here section.

    maflcko commented at 2:18 PM on July 4, 2022:

    Good find. Thanks and fixed.

  30. maflcko force-pushed on Jul 4, 2022
  31. maflcko commented at 2:19 PM on July 4, 2022: member

    Added "missing" lock to single-threaded bitcoin-chainstate.cpp.

    Should be easy re-ACK with git range-diff bitcoin-core/master fa35905c0a fadb84b05f.

  32. maflcko force-pushed on Jul 6, 2022
  33. maflcko commented at 4:41 PM on July 6, 2022: member

    Rebased due to silent and benign conflict.

  34. DrahtBot cross-referenced this on Jul 9, 2022 from issue Severity-based logging -- parent PR by jonatack
  35. maflcko force-pushed on Jul 12, 2022
  36. maflcko commented at 3:49 PM on July 12, 2022: member

    Rebased (for fun)

  37. maflcko force-pushed on Jul 12, 2022
  38. maflcko commented at 3:50 PM on July 12, 2022: member

    Reworked commits to make review easier

  39. maflcko commented at 8:01 AM on July 14, 2022: member

    This keeps happening in CI: https://cirrus-ci.com/task/6262715361525760?logs=ci#L5141

  40. michaelfolkson commented at 5:36 PM on July 15, 2022: contributor

    Concept ACK, Approach ACK

    I haven't been able to replicate the issues this PR resolves myself but convinced at the PR review club last week that they exist and this PR is the best way to resolve them.

    This keeps happening in CI: https://cirrus-ci.com/task/6262715361525760?logs=ci#L5141

    Sorry, what is "this" Marco? You link to a merge commit for a doc change? The following?

    "Scheduling was delayed due to a concurrency limit on community tasks Consider upgrading to a $10/month plan to get 2x the limits on any public and private repository"

  41. maflcko commented at 7:52 AM on July 16, 2022: member

    Sorry, what is "this" Marco? You link to a merge commit for a doc change? The following?

    This is a link to a CI log to line 5141 (see # in the URL), the excerpt is:

    feature_reindex.py                                     | ✖ Failed  | 5 s
ALL                                                    | ✖ Failed  | 1410 s (accumulated) 
Runtime: 181 s
==================
WARNING: ThreadSanitizer: data race (pid=31956)
  Read of size 8 at 0x7b3c000008f8 by main thread:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:46 (bitcoind+0x15382d)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15382d)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) ChainstateManager::ActiveTip() const src/./validation.h:918:59 (bitcoind+0x15382d)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1857:35 (bitcoind+0x15382d)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
  Previous write of size 8 at 0x7b3c000008f8 by thread T22 (mutexes: write M131617, write M151, write M131548):
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) std::__1::enable_if<(is_move_constructible<CBlockIndex**>::value) && (is_move_assignable<CBlockIndex**>::value), void>::type std::__1::swap<CBlockIndex**>(CBlockIndex**&, CBlockIndex**&) /usr/lib/llvm-13/bin/../include/c++/v1/__utility/swap.h:39:7 (bitcoind+0x52c116)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__swap_out_circular_buffer(std::__1::__split_buffer<CBlockIndex*, std::__1::allocator<CBlockIndex*>&>&) /usr/lib/llvm-13/bin/../include/c++/v1/vector:978:5 (bitcoind+0x52c116)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::__append(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:1117:9 (bitcoind+0x52c116)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::resize(unsigned long) /usr/lib/llvm-13/bin/../include/c++/v1/vector:2046:15 (bitcoind+0x52acf9)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x52acf9)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2739:13 (bitcoind+0x49f9e5)
    [#6](/github-metadata-backup-bitcoin-bitcoin/6/) CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2875:18 (bitcoind+0x4a108e)
    [#7](/github-metadata-backup-bitcoin-bitcoin/7/) CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3002:22 (bitcoind+0x4a186f)
    [#8](/github-metadata-backup-bitcoin-bitcoin/8/) node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x235d74)
    [#9](/github-metadata-backup-bitcoin-bitcoin/9/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1673:9 (bitcoind+0x15a68e)
    [#10](/github-metadata-backup-bitcoin-bitcoin/10/) decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15a68e)
    [#11](/github-metadata-backup-bitcoin-bitcoin/11/) void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15a68e)
    [#12](/github-metadata-backup-bitcoin-bitcoin/12/) std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15a68e)
    [#13](/github-metadata-backup-bitcoin-bitcoin/13/) std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15a68e)
    [#14](/github-metadata-backup-bitcoin-bitcoin/14/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x8d917f)
    [#15](/github-metadata-backup-bitcoin-bitcoin/15/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x8d917f)
    [#16](/github-metadata-backup-bitcoin-bitcoin/16/) util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x8d917f)
    [#17](/github-metadata-backup-bitcoin-bitcoin/17/) decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x159eba)
    [#18](/github-metadata-backup-bitcoin-bitcoin/18/) void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x159eba)
    [#19](/github-metadata-backup-bitcoin-bitcoin/19/) void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x159eba)
  Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) operator new(unsigned long) <null> (bitcoind+0x1342b8)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4844:21 (bitcoind+0x4b818b)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) node::LoadChainstate(bool, ChainstateManager&, CTxMemPool*, bool, bool, long, long, long, bool, bool, std::__1::function<bool ()>, std::__1::function<void ()>) src/node/chainstate.cpp:30:14 (bitcoind+0x246e48)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1457:32 (bitcoind+0x150ad2)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
  Mutex M131617 (0x7b3c00000898) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_trylock <null> (bitcoind+0xc0c18)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::mutex::try_lock() <null> (libc++.so.1+0x49f55)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) UniqueLock<AnnotatedMixin<std::__1::mutex>, std::__1::unique_lock<std::__1::mutex> >::UniqueLock(AnnotatedMixin<std::__1::mutex>&, char const*, char const*, int, bool) src/./sync.h:181:13 (bitcoind+0x4a15a0)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:2966:5 (bitcoind+0x4a15a0)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) node::ThreadImport(ChainstateManager&, std::__1::vector<fs::path, std::__1::allocator<fs::path> >, ArgsManager const&) src/node/blockstorage.cpp:883:30 (bitcoind+0x235d74)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1673:9 (bitcoind+0x15a68e)
    [#6](/github-metadata-backup-bitcoin-bitcoin/6/) decltype(static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(fp)()) std::__1::__invoke<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x15a68e)
    [#7](/github-metadata-backup-bitcoin-bitcoin/7/) void std::__1::__invoke_void_return_wrapper<void, true>::__call<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&>(AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&) /usr/lib/llvm-13/bin/../include/c++/v1/__functional/invoke.h:61:9 (bitcoind+0x15a68e)
    [#8](/github-metadata-backup-bitcoin-bitcoin/8/) std::__1::__function::__alloc_func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:171:16 (bitcoind+0x15a68e)
    [#9](/github-metadata-backup-bitcoin-bitcoin/9/) std::__1::__function::__func<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, std::__1::allocator<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>, void ()>::operator()() /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:345:12 (bitcoind+0x15a68e)
    [#10](/github-metadata-backup-bitcoin-bitcoin/10/) std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x8d917f)
    [#11](/github-metadata-backup-bitcoin-bitcoin/11/) std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x8d917f)
    [#12](/github-metadata-backup-bitcoin-bitcoin/12/) util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x8d917f)
    [#13](/github-metadata-backup-bitcoin-bitcoin/13/) decltype(static_cast<void (*>(fp)(static_cast<char const*>(fp0), static_cast<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(fp0))) std::__1::__invoke<void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(char const*, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/type_traits:3918:1 (bitcoind+0x159eba)
    [#14](/github-metadata-backup-bitcoin-bitcoin/14/) void std::__1::__thread_execute<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-13/bin/../include/c++/v1/thread:280:5 (bitcoind+0x159eba)
    [#15](/github-metadata-backup-bitcoin-bitcoin/15/) void* std::__1::__thread_proxy<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct> >, void (*)(char const*, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7> >(void*) /usr/lib/llvm-13/bin/../include/c++/v1/thread:291:5 (bitcoind+0x159eba)
  Mutex M151 (0x55cf3ac0d3c0) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_init <null> (bitcoind+0xc097f)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) __libc_start_main <null> (libc.so.6+0x29eba)
  Mutex M131548 (0x7b5000000c60) created at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_mutex_init <null> (bitcoind+0xc097f)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::__unique_if<CTxMemPool>::__unique_single std::__1::make_unique<CTxMemPool, kernel::MemPoolOptions&>(kernel::MemPoolOptions&) /usr/lib/llvm-13/bin/../include/c++/v1/__memory/unique_ptr.h:728:32 (bitcoind+0x15e7a2)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1441:24 (bitcoind+0x1508c0)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
  Thread T22 'b-loadblk' (tid=32032, running) created by main thread at:
    [#0](/github-metadata-backup-bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0xbf20d)
    [#1](/github-metadata-backup-bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create(unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-13/bin/../include/c++/v1/__threading_support:443:10 (bitcoind+0x157fb6)
    [#2](/github-metadata-backup-bitcoin-bitcoin/2/) std::__1::thread::thread<void (*)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, void>(void (*&&)(char const*, std::__1::function<void ()>), char const (&) [8], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-13/bin/../include/c++/v1/thread:307:16 (bitcoind+0x157fb6)
    [#3](/github-metadata-backup-bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1672:29 (bitcoind+0x152237)
    [#4](/github-metadata-backup-bitcoin-bitcoin/4/) AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    [#5](/github-metadata-backup-bitcoin-bitcoin/5/) main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
SUMMARY: ThreadSanitizer: data race /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:46 in std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const
==================

    The CI ran Commit 8479ed0 on branch master.

  42. DrahtBot cross-referenced this on Jul 21, 2022 from issue assumeutxo: snapshot initialization by jamesob
  43. DrahtBot cross-referenced this on Jul 26, 2022 from issue refactor: Remove almost all validation option globals by maflcko
  44. michaelfolkson commented at 6:49 PM on July 27, 2022: contributor

    ACK fa547051bae19c7510e808d929f5d8df9a882758

    Built on MacOS Monterey, updated unit tests pass and reviewed code (not particularly qualified though on this kind of PR). As I said here I haven't replicated the issues either.

  45. DrahtBot cross-referenced this on Jul 29, 2022 from issue assumeutxo: background validation completion by jamesob
  46. maflcko added this to the milestone 24.0 on Aug 10, 2022
  47. achow101 commented at 7:32 PM on August 15, 2022: member

    ACK fa547051bae19c7510e808d929f5d8df9a882758

  48. achow101 commented at 7:45 PM on August 15, 2022: member

    Looks like a silent merge conflict with master:

    wallet/test/availablecoins_tests.cpp:47:90: error: calling function 'ActiveChain' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        wallet->SetLastBlockProcessed(wallet->GetLastBlockHeight() + 1, m_node.chainman->ActiveChain().Tip()->GetBlockHash());
                                                                                         ^
wallet/test/availablecoins_tests.cpp:50:64: error: calling function 'ActiveChain' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1};
                                                               ^
wallet/test/availablecoins_tests.cpp:50:118: error: calling function 'ActiveChain' requires holding mutex 'cs_main' exclusively [-Werror,-Wthread-safety-analysis]
        it->second.m_state = TxStateConfirmed{m_node.chainman->ActiveChain().Tip()->GetBlockHash(), m_node.chainman->ActiveChain().Height(), /*index=*/1};
                                                                                                                     ^
3 errors generated.
  49. Add ChainstateManager::GetMutex(), an alias for ::cs_main fa530bcb9c
  50. Fix UB/data-race in RPCNotifyBlockChange
    ActiveTip() is *not* thread-safe, as the required ::cs_main lock will be
released as ActiveChainstate() returns.

ActiveTip() is an alias for ActiveChainstate().m_chain.Tip(), so m_chain
may be involved in a data-race (UB).
    fa97a528d6
  51. Fix logical race in rest_getutxos
    Calling ActiveHeight() and ActiveTip() subsequently without holding the
::cs_main lock over both calls may result in a height that does not
correspond to the tip due to a race.

Fix this by holding the lock.
    fac15ff673
  52. refactor: Add lock annotations to Active* methods
    This is a refactor, putting the burden to think about thread safety to
the caller. Otherwise, there is a risk that the caller will assume
thread safety where none exists, as is evident in the previous two
commits.
    fac04cb6ba
  53. maflcko force-pushed on Aug 16, 2022
  54. maflcko commented at 3:29 PM on August 16, 2022: member

    Thanks, rebased to fix silent conflict. Should be trivial to re-ACK with range-diff

  55. DrahtBot cross-referenced this on Aug 16, 2022 from issue test: clean and extend availablecoins_tests coverage by furszy
  56. achow101 commented at 9:25 PM on August 16, 2022: member

    re-ACK fac04cb6ba1d032587bd02eab2247fd655a548cd

  57. theStack approved
  58. theStack commented at 1:49 PM on August 17, 2022: contributor

    Code-review ACK fac04cb6ba1d032587bd02eab2247fd655a548cd

  59. fanquake merged this on Aug 17, 2022
  60. fanquake closed this on Aug 17, 2022
  61. ryanofsky cross-referenced this on Aug 17, 2022 from issue refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky
  62. sidhujag referenced this in commit 16c3326541 on Aug 17, 2022
  63. maflcko deleted the branch on Feb 23, 2023
  64. Naviabheeman cross-referenced this on Jun 22, 2023 from issue Improve shared data locking and Synchronisation by Naviabheeman
  65. Fabcien referenced this in commit 6722d82641 on Oct 18, 2023
  66. Fabcien referenced this in commit 1d6aeef072 on Dec 21, 2023
  67. bitcoin locked this on Feb 23, 2024
Contributors
maflckoCrypt-iQDrahtBotpk-b2ajtownsjamesobRiahiamirrezamichaelfolksonachow101theStack
Labels
BugValidation

Milestone
24.0

Linked (view graph)
#1 JSON-RPC support for mobile devices ("ultra-lightweight" clients)#2 Long-term, safe, store-of-value#3 Encrypt wallet#4 Export/Import wallet in a human readable, future-proof format#5 Make the version number the protocol version and not the client version#6 Treat wallet as a generic keystore#7 Block-header-only, faster startup client#8 RPC command to sign text with wallet private key#9 Fix for GUI on Macs and latest wxWidgets#10 Add address to listtransactions output#11 Nolisten patch#12 Monitor transactions and/or blocks#13 Messages with or about transactions#14 bitcoin: URI and/or bitcoin-request MIME type for click-to-pay#15 Option to specify external IP address#16 Mac UI issues#17 listaccounts method#18 Error when trying to send very precise amount#19 bitcoin(d) --help should output version number#25330 How to avoid logic races (and recursive mutexes)?#25862 refactor, kernel: Remove gArgs accesses from dbwrapper and txdb
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-05-19 06:53 UTC