wallet: Remove WalletDatabase refcounting and enforce only one Batch access the database at a time #20096

1. 
   achow101 commented at 10:31 pm on October 6, 2020: member

   We never have multiple threads making able to multiple WalletBatchs due to the extensive use of cs_wallet, cs_KeyStore, and cs_desc_man, so the use of refcounting to handle the case where that may happen is unnecessary. Thus we can use an assert to enforce that only one BerkeleyBatch is accessing the BerkeleyDatabase at a time. The only instances where multiple BerkeleyBatchs were being created were where one was made in a caller function, and then the called function created another for itself. For those cases, we instead either pass in the caller’s WalletBatch or limit the scope of the caller’s WalletBatch so that there are no overlaps.

   The refcounting in BerkeleyDatabase is kept because it did more than just count the number of BerkeleyBatchs, it also indicated when the database was flushed. However the AddRef and RemoveRef functions have been removed from WalletDatabase.

2. fanquake added the label Wallet on Oct 6, 2020
3. 
   DrahtBot commented at 2:09 am on October 7, 2020: 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:

   • #23414 (wallet: Fix comment grammar in bdb.h by zealsham)

   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.

4. DrahtBot added the label Needs rebase on Oct 14, 2020
5. achow101 force-pushed on Oct 14, 2020
6. DrahtBot removed the label Needs rebase on Oct 14, 2020
7. achow101 force-pushed on Nov 10, 2020
8. achow101 force-pushed on Nov 10, 2020
9. achow101 force-pushed on Nov 10, 2020
10. DrahtBot added the label Needs rebase on Dec 2, 2020
11. achow101 force-pushed on Dec 3, 2020
12. DrahtBot removed the label Needs rebase on Dec 3, 2020
13. achow101 force-pushed on Jan 13, 2021
14. achow101 force-pushed on Jan 13, 2021
15. in src/wallet/bdb.cpp:811 in 2669bae2db outdated

    790@@ -791,6 +791,7 @@ void BerkeleyDatabase::AddRef()
    791     } else {
    792         m_refcount++;
    793     }
    794+    assert(m_refcount <= 1);
    

    ---

    ---

    

    adamjonas commented at 3:46 pm on January 14, 2021:

    this is failing for me: AssertionError: Unexpected stderr Assertion failed: (m_refcount <= 1), function AddRef, file wallet/bdb.cpp, line 794. !=
16. DrahtBot added the label Needs rebase on Mar 12, 2021
17. achow101 force-pushed on Mar 12, 2021
18. DrahtBot removed the label Needs rebase on Mar 13, 2021
19. DrahtBot added the label Needs rebase on Jun 30, 2021
20. Add SetWalletFlagWithDB a72d637e91
21. Pass in WalletBatch to UpgradeKeyMetadata c61037c2b7
22. Pass in WalletBatch to UpgradeDescriptorCache a15c2ca439
23. Limit the scope of WalletBatch in NewKeyPool

    Make sure the WalletBatch in NewKeyPool writes and destructs before
    TopUp.

    271e768312
24. Have LearnRelatedScripts and LearnAllRelatedScripts take WalletBatch 97eff9a4e8
25. Limit scope of WalletBatch in SetupDescriptorGeneration edc201e3ac
26. Add RemoveWatchOnlyWithDB 59b2162ea3
27. Enforce that only one BerkeleyBatch exists at a time 9a9a5dbfcb
28. Remove AddRef and RemoveRef from WalletDatabase a4e761a5d1
29. achow101 force-pushed on Jul 1, 2021
30. DrahtBot removed the label Needs rebase on Jul 1, 2021
31. 
    promag commented at 7:37 am on October 19, 2021: member

    Concept ACK, Improves/simplifies the codebase by dropping useless support for concurrent batches.

    Do you want to rebase to get a fresh CI?

32. 
    MarcoFalke commented at 8:24 am on October 19, 2021: member

    It failed with

    0�[0;32m node1 stderr bitcoind: wallet/bdb.cpp:811: void BerkeleyDatabase::AddRef(): Assertion `m_refcount <= 1' failed. �[0m
    

    So I don’t think this is a CI issues that can be fixed by a rebase

33. 
    promag commented at 8:27 am on October 19, 2021: member

    Ok, fix and rebase 🙉
34. DrahtBot added the label Needs rebase on Nov 9, 2021
35. 
    DrahtBot commented at 2:57 pm on November 9, 2021: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

36. 
    achow101 commented at 9:12 pm on November 9, 2021: member

    Closing as the constraint is no longer true due to nesting.
37. achow101 closed this on Nov 9, 2021

    ---

38. DrahtBot locked this on Nov 9, 2022

Contributors
achow101 DrahtBot adamjonas promag MarcoFalke

Labels
Wallet Needs rebase