Add harmless missing cs_wallet lock in qt CoinControlDialog #10340

pull TheBlueMatt wants to merge 1 commits into bitcoin:master from TheBlueMatt:2017-05-fix-mapwallet-zap-runtime changing 3 files +137 −126

TheBlueMatt commented at 8:47 PM on May 4, 2017: member

This is only barely a missing lock - to tickle it you have to delete from mapWallet while populating your coin control dialog. The only way (AFAIK) to do so is to call removeprunedfunds.

Add harmless missing cs_wallet lock in qt CoinControlDialog

This is only barely a missing lock - to tickle it you have to
delete from mapWallet while populating your coin control dialog.
The only way (AFAIK) to do so is to call removeprunedfunds.

31b7171cbf

ryanofsky commented at 8:56 PM on May 4, 2017: member

utACK 31b7171cbfef7d66957a7721aff524778dbdf5b0
ryanofsky commented at 10:15 PM on May 4, 2017: member

It'd be nice to have a comment attached to cs_wallet saying when the lock needs to be acquired:

https://github.com/bitcoin/bitcoin/blob/d3dce0eb67e8691745be6cf7e04b920f5e419bd9/src/wallet/wallet.h#L721

Bringing this up because I think at this point if anybody would know how to write such a comment, it would probably be you.
fanquake added the label Refactoring on May 5, 2017
in src/qt/coincontroldialog.cpp:454 in 31b7171cbf
```
 465 | -        }
 466 |  
 467 | -        // Quantity
 468 | -        nQuantity++;
 469 | +    {
 470 | +        LOCK2(cs_main, model->wallet->cs_wallet);
```
jonasschnelli commented at 7:49 AM on May 5, 2017:

How can we avoid this really undesirable layer violation (model->wallet->cs_wallet)? This seems to go in the wrong direction with @ryanofsky approach to form clean layers for later process separation.

ryanofsky commented at 9:14 AM on May 5, 2017:

How can we avoid this really undesirable layer violation (model->wallet->cs_wallet)?

#10244 stops using CWalletTx pointers in Qt altogether, so I think the layer violation would only be temporary. I think #10340 complements #10244 because it makes sense to fix correctness and safety issues first, and then organize the code around those changes.

in src/qt/walletmodel.h:218 in 31b7171cbf

 214 | @@ -215,8 +215,9 @@ class WalletModel : public QObject
 215 |  
 216 |      bool getDefaultWalletRbf() const;
 217 |  
 218 | +    CWallet *wallet; // Used to lock cs_wallet for getOutputs/listCoins

jonasschnelli commented at 7:49 AM on May 5, 2017:

Meh!

jonasschnelli commented at 7:51 AM on May 5, 2017: contributor

If a non-wallet-model code-part needs to obtain the cs_wallet lock, then we should move the logic-part (that requires the lock) to wallet-model.
TheBlueMatt commented at 2:46 PM on May 5, 2017: member

Closing in favor of #10244, which is a much better fix, though larger diff.
TheBlueMatt closed this on May 5, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels