getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs

practicalswift commented at 1:15 pm on May 15, 2019: contributor

LockAnnotation lock(::cs_main) is a guarantee to the compiler thread analysis that ::cs_main is locked (when it couldn’t be determined otherwise).

Despite being annotated with the locking guarantee …

https://github.com/bitcoin/bitcoin/blob/65526fc8666fef35ef908dbc225f706bef642c7e/src/interfaces/chain.cpp#L134-L138

… getTipLocator() reads chainActive (via ::ChainActive()) without holding cs_main.

This can be verified by adding the following AssertLockHeld(cs_main):

 0$ git diff
 1diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
 2index 59623284d..9fc693a0f 100644
 3--- a/src/interfaces/chain.cpp
 4+++ b/src/interfaces/chain.cpp
 5@@ -134,6 +134,7 @@ class LockImpl : public Chain::Lock
 6     CBlockLocator getTipLocator() override
 7     {
 8         LockAnnotation lock(::cs_main);
 9+        AssertLockHeld(::cs_main);
10         return ::ChainActive().GetLocator();
11     }
12     Optional<int> findLocatorFork(const CBlockLocator& locator) override
13$ make check
14../build-aux/test-driver: line 107: 12881 Aborted                 "$@" > $log_file 2>&1
15FAIL: qt/test/test_bitcoin-qt

ryanofsky commented at 2:25 pm on May 15, 2019: member

It’d be useful to see a stack trace where the assert fails. Probably this is due to an assumeLocked() usage, and at this point I think we can pretty easily remove the assumeLocked method, which was supposed to be temporary. The last hard to replace assumeLocked usage was removed in 52b760fc6a9b26e405a0553ee8285b0f03ca1604 in #15632.

MarcoFalke added this to the milestone 0.19.0 on May 15, 2019

practicalswift commented at 2:52 pm on May 15, 2019: contributor

@ryanofsky Here goes:

 0Assertion failed: lock ::cs_main not held in interfaces/chain.cpp:137; locks held:
 1
 2…
 3(gdb) bt
 4[#0](/bitcoin-bitcoin/0/)  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
 5[#1](/bitcoin-bitcoin/1/)  0x00007ffff2d23801 in __GI_abort () at abort.c:79
 6[#2](/bitcoin-bitcoin/2/)  0x0000555555aa7cfd in AssertLockHeldInternal (pszName=0x555556106524 "::cs_main", pszFile=0x5555561063b2 "interfaces/chain.cpp", nLine=137, cs=<optimized out>) at sync.cpp:175
 7[#3](/bitcoin-bitcoin/3/)  0x0000555555774537 in interfaces::(anonymous namespace)::LockImpl::getTipLocator (this=<optimized out>) at interfaces/chain.cpp:137
 8[#4](/bitcoin-bitcoin/4/)  0x0000555555b53630 in CWallet::CreateWalletFromFile (chain=..., location=..., wallet_creation_flags=0) at wallet/wallet.cpp:4078
 9[#5](/bitcoin-bitcoin/5/)  0x0000555555b07523 in LoadWallets (chain=..., wallet_files=...) at wallet/load.cpp:68
10[#6](/bitcoin-bitcoin/6/)  0x0000555555ad383e in interfaces::(anonymous namespace)::WalletClientImpl::load (this=<optimized out>) at interfaces/wallet.cpp:521
11[#7](/bitcoin-bitcoin/7/)  0x000055555579282b in AppInitMain (interfaces=...) at init.cpp:1681
12[#8](/bitcoin-bitcoin/8/)  0x000055555577c8d7 in interfaces::(anonymous namespace)::NodeImpl::appInitMain (this=<optimized out>) at interfaces/node.cpp:78
13[#9](/bitcoin-bitcoin/9/)  0x000055555564b0ec in BitcoinCore::initialize (this=0x555556f2cc60) at qt/bitcoin.cpp:154

MarcoFalke added the label GUI on May 15, 2019

MarcoFalke renamed this:
~~getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee~~
gui: getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee
on May 15, 2019

practicalswift commented at 3:11 pm on May 15, 2019: contributor

@ryanofsky Like this?

 0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
 1index 69a78f1fc..44db2f29b 100644
 2--- a/src/wallet/test/wallet_tests.cpp
 3+++ b/src/wallet/test/wallet_tests.cpp
 4@@ -387,7 +387,7 @@ public:
 5     }
 6
 7     std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain();
 8-    std::unique_ptr<interfaces::Chain::Lock> m_locked_chain = m_chain->assumeLocked();  // Temporary. Removed in upcoming lock cleanup
 9+    std::unique_ptr<interfaces::Chain::Lock> m_locked_chain = m_chain->lock();
10     std::unique_ptr<CWallet> wallet;
11 };
12
13diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
14index 054329fbd..0822c7c30 100644
15--- a/src/wallet/wallet.cpp
16+++ b/src/wallet/wallet.cpp
17@@ -4074,7 +4074,7 @@ std::shared_ptr<CWallet> CWallet::CreateWalletFromFile(interfaces::Chain& chain,
18             return nullptr;
19         }
20
21-        auto locked_chain = chain.assumeLocked();  // Temporary. Removed in upcoming lock cleanup
22+        auto locked_chain = chain.lock();
23         walletInstance->ChainStateFlushed(locked_chain->getTipLocator());
24     } else if (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS) {
25         // Make it impossible to disable private keys after creation

ryanofsky commented at 3:21 pm on May 15, 2019: member

@ryanofsky Like this?

Yes, I think that should work. As followup, the assumeLocked method declarations and definitions could also be removed, the LockingStateImpl class could be deleted, and LockImpl could inherit directly from UniqueLock.

MarcoFalke removed the label GUI on May 15, 2019

MarcoFalke renamed this:
~~gui: getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee~~
getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee
on May 15, 2019

practicalswift commented at 8:50 am on May 16, 2019: contributor

Fix in #16033.

MarcoFalke closed this on May 17, 2019

MarcoFalke referenced this in commit f3d27d126b on May 17, 2019

MarcoFalke referenced this in commit 65c4bbe629 on May 23, 2019

sidhujag referenced this in commit 7fb5ee1cab on May 24, 2019

jasonbcox referenced this in commit e942ee183d on Jul 2, 2020

DrahtBot locked this on Dec 16, 2021

getTipLocator() reads chainActive without holding cs_main despite LockAnnotation lock(::cs_main) guarantee #16028