wallet: Avoid multiple BerkeleyBatch in DelAddressBook #19738

1. 
   promag commented at 11:09 pm on August 16, 2020: member
2. DrahtBot added the label Wallet on Aug 16, 2020
3. 
   meshcollider commented at 3:24 am on August 17, 2020: contributor

   Code review ACK b2ce2b97501f8642b79da024dd485545b67f5533
4. 
   achow101 commented at 5:08 pm on August 18, 2020: member

   In CWallet::FindNonChangeParentOutput, I think we should add an AssertLockHeld(cs_wallet) too.
5. 
   DrahtBot commented at 8:14 pm on August 20, 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:

   • #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)

   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.

6. wallet: Avoid multiple BerkeleyBatch in DelAddressBook abac436760
7. promag force-pushed on Sep 6, 2020
8. 
   promag commented at 10:00 am on September 6, 2020: member

   In CWallet::FindNonChangeParentOutput, I think we should add an AssertLockHeld(cs_wallet) too.

   Done. Also rebased now that #19289 is merged.

9. 
   achow101 commented at 5:28 pm on September 6, 2020: member

   ACK abac4367607d8d2b628e4db6a9663c960bacdacc
10. in src/wallet/wallet.cpp:2349 in abac436760

    2345@@ -2346,6 +2346,7 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
    2346 
    2347 const CTxOut& CWallet::FindNonChangeParentOutput(const CTransaction& tx, int output) const
    2348 {
    2349+    AssertLockHeld(cs_wallet);
    

    ---

    ---

    

    jonatack commented at 7:14 pm on September 6, 2020:

    @achow101 per #19738 (comment) I’m curious why this should be added, as there is already an AssertLockHeld(cs_wallet); at the top of its caller, ListCoins(). For a future call from elsewhere?

    ---

    

    promag commented at 7:28 pm on September 6, 2020:

    This was based on #19289 which then raised #19773, and @achow101 suggestion should be done there, ListCoins eventually calls FindNonChangeParentOutput, and like IsTrusted, was missing the assertion.

    ---

    

    promag commented at 7:45 pm on September 6, 2020:

    @jonatack I could split to a different commit though to be more correct, just LMK.

    ---

    

    jonatack commented at 7:55 pm on September 6, 2020:

    No worries, I was only trying to understand the locking. This is fine.
11. 
    jonatack commented at 7:18 pm on September 6, 2020: member

    ACK abac4367607d8d2b628e4db6a9663c960bacdacc
12. 
    meshcollider commented at 3:43 am on September 7, 2020: contributor

    re-utACK abac4367607d8d2b628e4db6a9663c960bacdacc
13. meshcollider merged this on Sep 7, 2020
14. meshcollider closed this on Sep 7, 2020

    ---

15. Fabcien referenced this in commit df5b53eeee on Sep 27, 2021
16. DrahtBot locked this on Feb 15, 2022

Contributors
promag meshcollider achow101 DrahtBot jonatack

Labels
Wallet