wallet: Avoid multiple BerkeleyBatch in DelAddressBook #19738

pull promag wants to merge 1 commits into bitcoin:master from promag:2020-08-deladdressbook changing 1 files +5 −3
  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

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-18 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me