Fix chain tip data race and corrupt rest response #25077

maflcko · 2022-05-06T14:25:41Z

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 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 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 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 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 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29)
    #5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00)
    #6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e)
    #7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e)
    #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f)
    #15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f)
    #16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f)
    #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 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 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 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d)
    #1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d)
    #2 ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d)
    #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread:
    #0 operator new(unsigned long) <null> (bitcoind+0x132668)
    #1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Mutex M131626 (0x7b3c00000898) created at:
    #0 pthread_mutex_lock <null> (bitcoind+0xda898)
    #1 std::__1::mutex::lock() <null> (libc++.so.1+0x49f35)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e)
    #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f)
    #9 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f)
    #10 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f)
    #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 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 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 pthread_mutex_init <null> (bitcoind+0xbed2f)
    #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    #2 __libc_start_main <null> (libc.so.6+0x29eba)
  Mutex M131553 (0x7b4c000042e0) created at:
    #0 pthread_mutex_init <null> (bitcoind+0xbed2f)
    #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2)
  Thread T22 'b-loadblk' (tid=32370, running) created by main thread at:
    #0 pthread_create <null> (bitcoind+0xbd5bd)
    #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 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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2)
    #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

Crypt-iQ · 2022-05-06T14:52:57Z

src/init.cpp

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

Nevermind, I see the WITH_LOCK. mb

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.

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

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?

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

DrahtBot · 2022-05-06T19:57:13Z

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

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.

pk-b2

ACK fa35585c74ca48d622d0b2fb9bbfb1c2148eeb00

Github-Pull: bitcoin#25077 Rebased-From: fa35585c74ca48d622d0b2fb9bbfb1c2148eeb00

ajtowns · 2022-06-13T15:59:35Z

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?

maflcko · 2022-06-13T16:01:29Z

@DrahtBot Bad bot!!

maflcko · 2022-06-21T13:39:09Z

Rebased, answered question, and started removing ::cs_main

jamesob

Concept ACK

jamesob · 2022-06-21T14:21:24Z

src/validation.h

Nice, this is a good direction I think. 👍

Crypt-iQ

crACK fa35905c0a5d45ab16283536fb98124db71ef897

src/validation.h

maflcko · 2022-07-04T14:19:12Z

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

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

maflcko · 2022-07-06T16:41:44Z

Rebased due to silent and benign conflict.

maflcko · 2022-07-12T15:49:28Z

Rebased (for fun)

maflcko · 2022-07-12T15:50:32Z

Reworked commits to make review easier

maflcko · 2022-07-14T08:01:40Z

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

michaelfolkson · 2022-07-15T17:36:18Z

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"

maflcko · 2022-07-16T07:52:14Z

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 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:46 (bitcoind+0x15382d)
    #1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15382d)
    #2 ChainstateManager::ActiveTip() const src/./validation.h:918:59 (bitcoind+0x15382d)
    #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1857:35 (bitcoind+0x15382d)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    #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 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 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 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 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 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x52acf9)
    #5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2739:13 (bitcoind+0x49f9e5)
    #6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2875:18 (bitcoind+0x4a108e)
    #7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3002:22 (bitcoind+0x4a186f)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1673:9 (bitcoind+0x15a68e)
    #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x8d917f)
    #15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x8d917f)
    #16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x8d917f)
    #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 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 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 operator new(unsigned long) <null> (bitcoind+0x1342b8)
    #1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4844:21 (bitcoind+0x4b818b)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1457:32 (bitcoind+0x150ad2)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    #5 main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
  Mutex M131617 (0x7b3c00000898) created at:
    #0 pthread_mutex_trylock <null> (bitcoind+0xc0c18)
    #1 std::__1::mutex::try_lock() <null> (libc++.so.1+0x49f55)
    #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 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:2966:5 (bitcoind+0x4a15a0)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1673:9 (bitcoind+0x15a68e)
    #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x8d917f)
    #11 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x8d917f)
    #12 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x8d917f)
    #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 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 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 pthread_mutex_init <null> (bitcoind+0xc097f)
    #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    #2 __libc_start_main <null> (libc.so.6+0x29eba)
  Mutex M131548 (0x7b5000000c60) created at:
    #0 pthread_mutex_init <null> (bitcoind+0xc097f)
    #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3)
    #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1441:24 (bitcoind+0x1508c0)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    #5 main src/bitcoind.cpp:278:13 (bitcoind+0x135c99)
  Thread T22 'b-loadblk' (tid=32032, running) created by main thread at:
    #0 pthread_create <null> (bitcoind+0xbf20d)
    #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 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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1672:29 (bitcoind+0x152237)
    #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:234:43 (bitcoind+0x135c99)
    #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.

michaelfolkson · 2022-07-27T18:49:01Z

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.

achow101 · 2022-08-15T19:32:26Z

ACK fa547051bae19c7510e808d929f5d8df9a882758

achow101 · 2022-08-15T19:45:52Z

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.

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

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.

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.

maflcko · 2022-08-16T15:29:20Z

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

achow101 · 2022-08-16T21:25:17Z

re-ACK fac04cb

theStack

Code-review ACK fac04cb

fac04cb refactor: Add lock annotations to Active* methods (MacroFake) fac15ff Fix logical race in rest_getutxos (MacroFake) fa97a52 Fix UB/data-race in RPCNotifyBlockChange (MacroFake) fa530bc Add ChainstateManager::GetMutex(), an alias for ::cs_main (MacroFake) Pull request description: 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 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 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 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 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 CChain::SetTip(CBlockIndex*) src/chain.cpp:19:12 (bitcoind+0x4ffe29) #5 CChainState::ConnectTip(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, ConnectTrace&, DisconnectedBlockTransactions&) src/validation.cpp:2748:13 (bitcoind+0x475d00) #6 CChainState::ActivateBestChainStep(BlockValidationState&, CBlockIndex*, std::__1::shared_ptr<CBlock const> const&, bool&, ConnectTrace&) src/validation.cpp:2884:18 (bitcoind+0x47739e) #7 CChainState::ActivateBestChain(BlockValidationState&, std::__1::shared_ptr<CBlock const>) src/validation.cpp:3011:22 (bitcoind+0x477baf) #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #15 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #16 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #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 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 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 std::__1::vector<CBlockIndex*, std::__1::allocator<CBlockIndex*> >::size() const /usr/lib/llvm-13/bin/../include/c++/v1/vector:680:61 (bitcoind+0x15179d) #1 CChain::Tip() const src/./chain.h:449:23 (bitcoind+0x15179d) #2 ChainstateManager::ActiveTip() const src/./validation.h:927:59 (bitcoind+0x15179d) #3 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1841:35 (bitcoind+0x15179d) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Location is heap block of size 232 at 0x7b3c00000870 allocated by main thread: #0 operator new(unsigned long) <null> (bitcoind+0x132668) #1 ChainstateManager::InitializeChainstate(CTxMemPool*, std::__1::optional<uint256> const&) src/validation.cpp:4851:21 (bitcoind+0x48e26b) #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1438:32 (bitcoind+0x14e994) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Mutex M131626 (0x7b3c00000898) created at: #0 pthread_mutex_lock <null> (bitcoind+0xda898) #1 std::__1::mutex::lock() <null> (libc++.so.1+0x49f35) #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7::operator()() const src/init.cpp:1657:9 (bitcoind+0x15863e) #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 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 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 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 std::__1::__function::__value_func<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:498:16 (bitcoind+0x88891f) #9 std::__1::function<void ()>::operator()() const /usr/lib/llvm-13/bin/../include/c++/v1/__functional/function.h:1175:12 (bitcoind+0x88891f) #10 util::TraceThread(char const*, std::__1::function<void ()>) src/util/thread.cpp:18:9 (bitcoind+0x88891f) #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 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 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 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #2 __libc_start_main <null> (libc.so.6+0x29eba) Mutex M131553 (0x7b4c000042e0) created at: #0 pthread_mutex_init <null> (bitcoind+0xbed2f) #1 std::__1::recursive_mutex::recursive_mutex() <null> (libc++.so.1+0x49fb3) #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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1426:24 (bitcoind+0x14e7b4) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #5 main src/bitcoind.cpp:275:13 (bitcoind+0x133fd2) Thread T22 'b-loadblk' (tid=32370, running) created by main thread at: #0 pthread_create <null> (bitcoind+0xbd5bd) #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 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 AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) src/init.cpp:1656:29 (bitcoind+0x150164) #4 AppInit(node::NodeContext&, int, char**) src/bitcoind.cpp:231:43 (bitcoind+0x133fd2) #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 ACKs for top commit: achow101: re-ACK fac04cb theStack: Code-review ACK fac04cb Tree-SHA512: 9d619f99ff6373874c7ffe1db20674575605646b4b54b692fb54515a4a49f110a770026d7320ed6dfeaa7976be4cd89e93f821acdbf22c7662bd1c5be0cedcd2

Summary: bitcoin/bitcoin@fa530bc > Add ChainstateManager::GetMutex(), an alias for ::cs_main bitcoin/bitcoin@fa97a52 > 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). bitcoin/bitcoin@fac15ff > 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. bitcoin/bitcoin@fac04cb > 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. This is a backport of [[bitcoin/bitcoin#25077 | core#25077]] Depends on D14642 Test Plan: with debug & tsan `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D14643

Summary: The bitcoin-chainstate executable serves to surface the dependencies required by a program wishing to use Bitcoin Core's consensus engine as it is right now. More broadly, the _SOURCES list serves as a guiding "North Star" for the libbitcoinkernel project: as we decouple more and more modules of the codebase from our consensus engine, this _SOURCES list will grow shorter and shorter. One day, only what is critical to our consensus engine will remain. Right now, it's "the minimal list of files to link in to even use our consensus engine". The additional dependencies for Bitcoin ABC pulled in via validation -> avalanche are listed separately because they will take more than just backporting bitcoin Core to get rid of. [META] In a future commit the libbitcoinkernel library will be extracted from bitcoin-chainstate, and the libbitcoinkernel library's _SOURCES list will be the list that we aim to shrink. This is a backport of [[bitcoin/bitcoin#24304 | core#24304]] and [[bitcoin/bitcoin#24504 | core#24504]] with updates from [[bitcoin/bitcoin#24661 | core#24661]], [[bitcoin/bitcoin#25308 | core#25308]], [[bitcoin/bitcoin#22564 | core#22564]], [[bitcoin/bitcoin#24595 | core#24595]], [[bitcoin/bitcoin#25168 | core#25168]], [[bitcoin/bitcoin#25077 | core#25077]], [[bitcoin/bitcoin#24153 | core#24153]] Test Plan: ``` cmake .. -GNinja -DBUILD_BITCOIN_CHAINSTATE=ON ninja ``` Running the new executable and pasting some hex blocks (the ones just after the tip from our chainstate) ``` $ src/bitcoin-chainstate /data/ecashd Hello! I'm going to print out some information about your datadir. Path: "/data/ecashd" Reindexing: false Snapshot Active: false Active Height: 823235 Active IBD: true CBlockIndex(pprev=0x7fe1085ce0c8, nHeight=823235, merkle=6a3a479d92db7c5a178356c680a58ff0468ed2aaf689c850b55729ebb0038568, hashBlock=0000000000000000085542cbdfd82a970117d737ebe8bc7a5c04d78d80d7bce9) 00e01420e9bcd7808dd7045c7abce8eb37d71701972ad8dfcb42550800000000000000005b31c63b1d34d740df4dea822a57439cdcf9c34b7a07a47c65c7602b78e81978ec388165c5102f18b4e7455f0101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff5803c48f0c04ed3881650cfabe6d6db68846e753ced995dcfb5a40b17108c8aceee4f11bb86c48c31000d62cd8859b10000000000000002ffffbec16c3eb3e20000000153237616661356365343565313830636333343636380000000003a04f9b15000000001976a914ce8c8cf69a922a607e8e03e27ec014fbc24882e088ac00c2eb0b0000000017a914d37c4c809fe9840e7bfa77b86bd47163f6fb6c6087a0acb903000000001976a914c36941af4c8cdf6e3156f7fe1426d05d6177890e88ac00000000 Valid initial value. Block has not yet been rejected 0040382de2bda14474f926af7d0e202cb5cd4583b978fd58c42d4c1600000000000000008addc8d8119c2d3d4943b06d7e532aca3dc146aa563e72f347f34bf0a37a1d5a0639816500f52e18360673500101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff5803c58f0c04063981650cfabe6d6d3b4767d5cada5b02046d6405e06e30e589b2bdf8de43e1517b8bf59ff31aca1f10000000000000002ffffc3b38f5cc2904000000153237616661356365343565313830636333343637310000000003a04f9b15000000001976a914ce8c8cf69a922a607e8e03e27ec014fbc24882e088ac00c2eb0b0000000017a914d37c4c809fe9840e7bfa77b86bd47163f6fb6c6087a0acb903000000001976a914600fe1afb15029166355501cb7887426b0055ca188ac00000000 Valid initial value. Block has not yet been rejected ``` Re-running `bitcoin-chainstate`, note that the active tip has advanced by two blocks, then pasting an old block (duplicate) ``` $ src/bitcoin-chainstate /data/ecashd Hello! I'm going to print out some information about your datadir. Path: "/data/ecashd" Reindexing: false Snapshot Active: false Active Height: 823237 Active IBD: true CBlockIndex(pprev=0x7fdb16e3b0c8, nHeight=823237, merkle=5a1d7aa3f04bf347f3723e56aa46c13dca2a537e6db043493d2d9c11d8c8dd8a, hashBlock=00000000000000000571463c80aa2dd411dbb5733c84e80b13ab329066918a84) 00c0012014b97d29661368d02d2593437c6cfbced254bc93daebea1f0000000000000000d53f666733a961c0f6d54a17c6c1a0de50b8687873aeacb0c12590711b89145bb52f816547f72e18c649a0890101000000010000000000000000000000000000000000000000000000000000000000000000ffffffff5803c18f0c04b62f81650cfabe6d6dc0d0d8c1dda463e1b906bfd1df887f20647ce66b7bf71fcda76fe1a7a5a8b4b110000000000000005ffffd71c7682a7fc8000000156536303266303261396362393865623833343836380000000003a04f9b15000000001976a914ce8c8cf69a922a607e8e03e27ec014fbc24882e088ac00c2eb0b0000000017a914d37c4c809fe9840e7bfa77b86bd47163f6fb6c6087a0acb903000000001976a914798038c8969512b74e82124a9a7364192893237188ac00000000 duplicate ``` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D15015

DrahtBot added the Refactoring label May 6, 2022

Crypt-iQ reviewed May 6, 2022

View reviewed changes

pk-b2 approved these changes May 7, 2022

View reviewed changes

DrahtBot mentioned this pull request May 7, 2022

assumeutxo: add init and completion logic #24232

Closed

1 task

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request May 22, 2022

Fix chain tip data race and corrupt rest response

d300cd9

Github-Pull: bitcoin#25077 Rebased-From: fa35585c74ca48d622d0b2fb9bbfb1c2148eeb00

This was referenced May 25, 2022

CChainState -> Chainstate #24513

Merged

[kernel 2d/n] Reduce CTxMemPool constructor call sites #25215

Merged

This was referenced Jun 7, 2022

[kernel 3a/n] Decouple CTxMemPool from ArgsManager #25290

Merged

Add DataStream without ser-type and ser-version and use it where possible #25296

Closed

maflcko mentioned this pull request Jun 10, 2022

How to avoid logic races (and recursive mutexes)? #25330

Closed

maflcko added Bug and removed Refactoring labels Jun 13, 2022

maflcko added the Validation label Jun 13, 2022

DrahtBot added the Needs rebase label Jun 16, 2022

maflcko force-pushed the 2205-tip-race- branch 2 times, most recently from 038bf45 to fa35905 Compare June 21, 2022 13:34

jamesob reviewed Jun 21, 2022

View reviewed changes

src/validation.h Outdated

Copy link

Contributor

jamesob Jun 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, this is a good direction I think. 👍

otech47 reacted with thumbs up emoji

DrahtBot removed the Needs rebase label Jun 21, 2022

Crypt-iQ reviewed Jun 21, 2022

View reviewed changes

Riahiamirreza approved these changes Jul 3, 2022

View reviewed changes

ajtowns reviewed Jul 4, 2022

View reviewed changes

src/validation.h Outdated Show resolved Hide resolved

maflcko force-pushed the 2205-tip-race- branch from fa35905 to fadb84b Compare July 4, 2022 14:16

maflcko force-pushed the 2205-tip-race- branch from fadb84b to fa0f0ce Compare July 6, 2022 16:40

maflcko force-pushed the 2205-tip-race- branch from 6874bd2 to fa54705 Compare July 12, 2022 15:50

This was referenced Jul 21, 2022

assumeutxo: snapshot initialization #25667

Merged

refactor: Remove almost all validation option globals #25704

Merged

DrahtBot mentioned this pull request Jul 29, 2022

assumeutxo: background validation completion #25740

Merged

maflcko added this to the 24.0 milestone Aug 10, 2022

MacroFake added 4 commits August 16, 2022 17:25

Add ChainstateManager::GetMutex(), an alias for ::cs_main

fa530bc

Fix UB/data-race in RPCNotifyBlockChange

fa97a52

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

Fix logical race in rest_getutxos

fac15ff

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.

refactor: Add lock annotations to Active* methods

fac04cb

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.

maflcko force-pushed the 2205-tip-race- branch from fa54705 to fac04cb Compare August 16, 2022 15:27

DrahtBot mentioned this pull request Aug 16, 2022

test: clean and extend availablecoins_tests coverage #25789

Merged

theStack approved these changes Aug 17, 2022

View reviewed changes

fanquake merged commit a75b779 into bitcoin:master Aug 17, 2022

ryanofsky mentioned this pull request Aug 17, 2022

refactor, kernel: Remove gArgs accesses from dbwrapper and txdb #25862

Merged

maflcko deleted the 2205-tip-race-🤠 branch February 23, 2023 17:08

Naviabheeman mentioned this pull request Jun 22, 2023

Improve shared data locking and Synchronisation chaintope/tapyrus-core#256

Closed

bitcoin locked and limited conversation to collaborators Feb 23, 2024

Fix chain tip data race and corrupt rest response #25077

Fix chain tip data race and corrupt rest response #25077

Uh oh!

Conversation

maflcko commented May 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Crypt-iQ May 6, 2022

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ May 6, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko May 6, 2022

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ May 9, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

pk-b2 left a comment

Choose a reason for hiding this comment

Uh oh!

ajtowns commented Jun 13, 2022

Uh oh!

maflcko commented Jun 13, 2022

Uh oh!

maflcko commented Jun 21, 2022

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

jamesob Jun 21, 2022

Choose a reason for hiding this comment

Uh oh!

Crypt-iQ left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Jul 4, 2022

Uh oh!

maflcko commented Jul 6, 2022

Uh oh!

maflcko commented Jul 12, 2022

Uh oh!

maflcko commented Jul 12, 2022

Uh oh!

maflcko commented Jul 14, 2022

Uh oh!

michaelfolkson commented Jul 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jul 16, 2022

Uh oh!

michaelfolkson commented Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 commented Aug 15, 2022

Uh oh!

achow101 commented Aug 15, 2022

Uh oh!

maflcko commented Aug 16, 2022

Uh oh!

achow101 commented Aug 16, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

maflcko commented May 6, 2022 •

edited

Loading

DrahtBot commented May 6, 2022 •

edited

Loading

michaelfolkson commented Jul 15, 2022 •

edited

Loading

michaelfolkson commented Jul 27, 2022 •

edited

Loading