Fix chain tip data race and corrupt rest response #25077
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Crypt-iQ
reviewed
May 6, 2022
src/init.cpp
Outdated
| @@ -1628,7 +1628,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info) | |||
| // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly. | |||
| // No locking, as this happens before any background thread is started. | |||
| boost::signals2::connection block_notify_genesis_wait_connection; | |||
| if (chainman.ActiveChain().Tip() == nullptr) { | |||
| if (WITH_LOCK(::cs_main, return chainman.ActiveChain().Tip() == nullptr)) { | |||
There was a problem hiding this comment.
Should callers be using ActiveTip instead of ActiveChain().Tip() since I think Tip() itself doesn't require a mutex and could change?
There was a problem hiding this comment.
Nevermind, I see the WITH_LOCK. mb
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should the CChain functions that use vChain have the exclusive lock annotation?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I think adding a member mutex would be a good idea in the future
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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
approved these changes
May 7, 2022
1 task
luke-jr
pushed a commit
to luke-jr/bitcoin
that referenced
this pull request
May 21, 2022
Github-Pull: bitcoin#25077 Rebased-From: fa35585
luke-jr
pushed a commit
to bitcoinknots/bitcoin
that referenced
this pull request
May 22, 2022
Github-Pull: bitcoin#25077 Rebased-From: fa35585
This was referenced May 25, 2022
This was referenced Jun 7, 2022
|
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? |
|
@DrahtBot Bad bot!! |
038bf45 to
fa35905
Compare
|
Rebased, answered question, and started removing |
jamesob
reviewed
Jun 21, 2022
| * - Force call sites to think how long they need to acquire the mutex to | ||
| * get consistent results. | ||
| */ | ||
| RecursiveMutex& GetMutex() const LOCK_RETURNED(::cs_main) { return ::cs_main; } |
There was a problem hiding this comment.
Nice, this is a good direction I think. 👍
Crypt-iQ
reviewed
Jun 21, 2022
Riahiamirreza
approved these changes
Jul 3, 2022
ajtowns
reviewed
Jul 4, 2022
|
Added "missing" lock to single-threaded bitcoin-chainstate.cpp. Should be easy re-ACK with |
Rebased due to silent and benign conflict.
|
Rebased (for fun) |
|
Reworked commits to make review easier |
|
This keeps happening in CI: https://cirrus-ci.com/task/6262715361525760?logs=ci#L5141 |
|
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.
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 |
This is a link to a CI log to line 5141 (see |
This was referenced Jul 21, 2022
|
ACK fa54705 |
|
Looks like a silent merge conflict with master: |
added 4 commits
August 16, 2022 17:25
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.
fa54705 to
fac04cb
Compare
|
Thanks, rebased to fix silent conflict. Should be trivial to re-ACK with range-diff |
|
re-ACK fac04cb |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Aug 17, 2022
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) 3D11 #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
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Oct 18, 2023
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
Fabcien
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Dec 21, 2023
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two issues:
ActiveChain, which returns a reference to the chain (astd::vector), which is not thread safe. See also below traceback.The issues are fixed by taking cs_main and holding it for the required time.
From https://cirrus-ci.com/task/5612886578954240?logs=ci#L4868