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

promag commented at 11:09 PM on August 16, 2020: member

No description provided.
DrahtBot added the label Wallet on Aug 16, 2020
meshcollider commented at 3:24 AM on August 17, 2020: contributor

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

In CWallet::FindNonChangeParentOutput, I think we should add an AssertLockHeld(cs_wallet) too.
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.
wallet: Avoid multiple BerkeleyBatch in DelAddressBook abac436760
promag force-pushed on Sep 6, 2020
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.
achow101 commented at 5:28 PM on September 6, 2020: member

ACK abac4367607d8d2b628e4db6a9663c960bacdacc
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.
jonatack commented at 7:18 PM on September 6, 2020: member

ACK abac4367607d8d2b628e4db6a9663c960bacdacc
meshcollider commented at 3:43 AM on September 7, 2020: contributor

re-utACK abac4367607d8d2b628e4db6a9663c960bacdacc
meshcollider merged this on Sep 7, 2020
meshcollider closed this on Sep 7, 2020
Fabcien referenced this in commit df5b53eeee on Sep 27, 2021
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

wallet: Avoid multiple BerkeleyBatch in DelAddressBook #19738

Conflicts