Hold cs_main when reading chainActive via getTipLocator(). Remove assumeLocked(). #16033

1. 
   practicalswift commented at 8:49 am on May 16, 2019: contributor

   Fixes #16028.

   Problem description:

   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
   

2. fanquake added the label Refactoring on May 16, 2019
3. 
   ryanofsky commented at 12:18 pm on May 16, 2019: member

   Can you also delete the LockingStateImpl class? There is no reason for the LockingStateImpl class to exist without assumeLocked. You just need to s/LockingStateImpl/LockImpl/ in the lock method and make LockImpl inherit from UniqueLock.
4. 
   practicalswift commented at 1:50 pm on May 16, 2019: contributor

   @ryanofsky Thanks for the quick review!

   Do you mean like this?

    0diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
    1index 1a8c32ee3..eaddcd9dd 100644
    2--- a/src/interfaces/chain.cpp
    3+++ b/src/interfaces/chain.cpp
    4@@ -37,7 +37,7 @@
    5 namespace interfaces {
    6 namespace {
    7
    8-class LockImpl : public Chain::Lock
    9+class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
   10 {
   11     Optional<int> getHeight() override
   12     {
   13@@ -155,10 +155,7 @@ class LockImpl : public Chain::Lock
   14         return AcceptToMemoryPool(::mempool, state, tx, nullptr /* missing inputs */, nullptr /* txn replaced */,
   15             false /* bypass limits */, absurd_fee);
   16     }
   17-};
   18
   19-class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
   20-{
   21     using UniqueLock::UniqueLock;
   22 };
   23
   24@@ -249,10 +246,10 @@ class ChainImpl : public Chain
   25 public:
   26     std::unique_ptr<Chain::Lock> lock(bool try_lock) override
   27     {
   28-        auto result = MakeUnique<LockingStateImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
   29+        auto result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
   30         if (try_lock && result && !*result) return {};
   31         // std::move necessary on some compilers due to conversion from
   32-        // LockingStateImpl to Lock pointer
   33+        // LockImpl to Lock pointer
   34         return std::move(result);
   35     }
   36     bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
   

5. 
   ryanofsky commented at 2:07 pm on May 16, 2019: member

   Do you mean like this?

   Thanks, yes that looks perfect.

6. in src/wallet/test/wallet_tests.cpp:405 in 00f941055f outdated

   400@@ -399,8 +401,9 @@ BOOST_FIXTURE_TEST_CASE(ListCoins, ListCoinsTestingSetup)
   401     // address.
   402     std::map<CTxDestination, std::vector<COutput>> list;
   403     {
   404+        auto locked_chain = m_chain->lock();
   405         LOCK2(cs_main, wallet->cs_wallet);
   

   ---

   ---

   

   MarcoFalke commented at 3:32 pm on May 16, 2019:

   0        LOCK(wallet->cs_wallet);
   

7. 
   MarcoFalke commented at 3:34 pm on May 16, 2019: member

   Could squash all the refactoring except the change to /wallet/wallet.cpp, which could go into a separate commit?
8. 
   DrahtBot commented at 4:16 pm on May 16, 2019: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #16034 (Add assertion to make sure the LockAnnotation guarantees we give are truthful (ifdef DEBUG_LOCKORDER). Rename LockAnnotation to LockAssertion. by practicalswift)
   • #15921 (Tidy up ValidationState interface by jnewbery)
   • #15713 (refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard)

   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.

9. wallet: Use chain.lock() instead of temporary chain.assumeLocked() 593a8e8a2c
10. Remove temporary method assumeLocked(). Remove LockingStateImpl. Remove redundant cs_main locks. 9402ef0739
11. practicalswift force-pushed on May 16, 2019
12. 
    practicalswift commented at 7:46 pm on May 16, 2019: contributor

    @MarcoFalke Thanks for the quick review! Updated accordingly. Please re-review :-)
13. 
    MarcoFalke commented at 8:12 pm on May 16, 2019: member

    utACK 9402ef0739fdcd8e989c07c0595095e9608b243c
14. ryanofsky approved
15. 
    ryanofsky commented at 8:46 pm on May 16, 2019: member

    utACK 00f941055fedb855473d5428f54c9d79786f6e23
16. 
    practicalswift commented at 8:49 pm on May 16, 2019: contributor

    @ryanofsky Thanks for the review. I think you utACK:ed the previous commit hash :-)
17. ryanofsky approved
18. 
    ryanofsky commented at 10:05 pm on May 16, 2019: member

    utACK 9402ef0739fdcd8e989c07c0595095e9608b243c. Changes are consolidating commits and removing redundant lock2 cs_main calls
19. MarcoFalke merged this on May 17, 2019
20. MarcoFalke closed this on May 17, 2019

    ---

21. MarcoFalke referenced this in commit f3d27d126b on May 17, 2019
22. MarcoFalke referenced this in commit 65c4bbe629 on May 23, 2019
23. sidhujag referenced this in commit 7fb5ee1cab on May 24, 2019
24. deadalnix referenced this in commit 8a2bca9d85 on May 19, 2020
25. practicalswift deleted the branch on Apr 10, 2021
26. kittywhiskers referenced this in commit 8201ec5c53 on Nov 6, 2021
27. kittywhiskers referenced this in commit f30ca25d31 on Nov 30, 2021
28. kittywhiskers referenced this in commit e9429e130d on Dec 3, 2021
29. kittywhiskers referenced this in commit 593d0d2aee on Dec 4, 2021
30. kittywhiskers referenced this in commit 371f4f3b7b on Dec 6, 2021
31. kittywhiskers referenced this in commit 128103eb18 on Dec 8, 2021
32. kittywhiskers referenced this in commit cf30ff9873 on Dec 8, 2021
33. kittywhiskers referenced this in commit 24b5dd683c on Dec 8, 2021
34. kittywhiskers referenced this in commit 1310a024ae on Dec 11, 2021
35. kittywhiskers referenced this in commit c9177f75eb on Dec 12, 2021
36. kittywhiskers referenced this in commit ee4c097cd7 on Dec 12, 2021
37. kittywhiskers referenced this in commit 007ef7f82f on Dec 12, 2021
38. kittywhiskers referenced this in commit 00a51cd508 on Dec 12, 2021
39. kittywhiskers referenced this in commit 3ac1ee05fb on Dec 12, 2021
40. kittywhiskers referenced this in commit dcff2dc2b7 on Dec 12, 2021
41. kittywhiskers referenced this in commit 4bc0ae1ee2 on Dec 12, 2021
42. Munkybooty referenced this in commit 49329d863a on Feb 6, 2022
43. Munkybooty referenced this in commit d33ccc71ee on Feb 15, 2022
44. Munkybooty referenced this in commit 26144dffac on Apr 21, 2022
45. Munkybooty referenced this in commit cb21e05f09 on Apr 26, 2022
46. Munkybooty referenced this in commit 079827f80e on May 4, 2022
47. Munkybooty referenced this in commit f99bac35ad on May 10, 2022
48. Munkybooty referenced this in commit 50daf4dce9 on May 17, 2022
49. DrahtBot locked this on Aug 16, 2022

Contributors
practicalswift ryanofsky MarcoFalke DrahtBot

Labels
Refactoring