This PR includes 2 fixes:
prevent GUI LoadWallet handlers from crashing on startup when multiple handlers are attached, because the first handler takes ownership of the wallet unique pointer. Now every handler will receive its own unique pointer;
prevent showing a wallet twice in the GUI on startup due to a race with loadwallet.
Fixes #16937
Good catch! But why exactly is the fix in #16955 incorrect? It seems like a reasonable workaround to me as long as the wallet still shows up in the GUI (which I would expect if m_node.getWallets() is called later). But if the wallet doesn’t show up in the GUI then the bigger fix is needed.
This clone fix should work now, and is more elegant than the nullptr workaround, but wouldn’t work great with #10102 unless I changed it to use shared_ptrs internally or complicate lifetime management some other way to support cloning.
More ideally, I would delete uiInterface.LoadWallet and Chain::loadWallet entirely and just not use boost signals for this callback. Instead I’d define a load wallet callback list (std::list<LoadWalletFn> load_wallet_fns) alongside the existing list of wallets (std::vector<std::shared_ptr<CWallet>> vpwallets), and push onto the list when handleLoadWallet is called, erase from the list when Handler::disconnect is called, and the replace existing line:
0chain.loadWallet(interfaces::MakeWallet(walletInstance));
with
0for (auto& load_wallet : load_wallet_fns) {
1 load_wallet(interfaces::MakeWallet(walletInstance));
2}
But I’m ok with the nullptr fix or the clone fix or however you want to proceed here.
std::move.
which I would expect if m_node.getWallets() is called later
There’s nothing guaranteeing it (I think?)
I started playing around with this change (7162be2eb2698eeea1eaaca2159d830acca63a1e). Using the same approach as in #16937 I was able to (only once so far) end up with the same wallet loaded twice:
Multiple times I’ve seen bitcoin-qt crash during load with:
0(lldb) run
1Process 20318 launched: '/Users/michael/github/bitcoin/src/qt/bitcoin-qt' (x86_64)
22019-09-26 20:14:33.701294+0800 bitcoin-qt[20318:1442147] MessageTracer: Falling back to default whitelist
3Process 20318 stopped
4* thread [#1](/bitcoin-bitcoin/1/), name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
5 frame [#0](/bitcoin-bitcoin/0/): 0x000000010009182e bitcoin-qt`GUIUtil::TableViewLastColumnResizingFixer::disconnectViewHeadersSignals(this=0x0000000000000000) at guiutil.cpp:445:16
6 442 // We need to disconnect these while handling the resize events, otherwise we can enter infinite loops.
7 443 void TableViewLastColumnResizingFixer::disconnectViewHeadersSignals()
8 444 {
9-> 445 disconnect(tableView->horizontalHeader(), &QHeaderView::sectionResized, this, &TableViewLastColumnResizingFixer::on_sectionResized);
10 446 disconnect(tableView->horizontalHeader(), &QHeaderView::geometriesChanged, this, &TableViewLastColumnResizingFixer::on_geometriesChanged);
11 447 }
12 448
13Target 0: (bitcoin-qt) stopped.
Ran out of time tonight, so will keep looking tomorrow.
48@@ -49,6 +49,9 @@ class Wallet
49 public:
50 virtual ~Wallet() {}
51
52+ //! Creates a copy of this interface implemantion.
Thanks @fanquake!
I was able to (only once so far) end up with the same wallet loaded twice
Very strange, I wonder if the “big” wallet has 2 WalletModel or not. I would suspect it is a bug on the WalletView creation, not on WalletController::getOrCreateWallet since it’s guarded by a mutex.
Edit: problem: 2 calls to BitcoinGUI::addWallet with the same wallet model are made because of:
https://github.com/bitcoin/bitcoin/blob/6288f15f50ab98b336ab86804c4cbea3807dc1d3/src/qt/bitcoingui.cpp#L623-L627
and if the dynamic loading happens after L623 and before L627.
that happens due to loadwallet? Or have you added other calls to the test script?
That was happening with just the loadwallet calls. However I have not seen either issue while testing 40660302d6a38ff0c26ecd80777990332a574c78 so far.
rebase --autosquash.
re: #16963 (comment)
More ideally, I would delete
uiInterface.LoadWallet
I haven’t seen any response to this suggestion, so I pushed a quick untested implementation to https://github.com/bitcoin/bitcoin/compare/master...ryanofsky:pr/loadwallets
I think it would be a better solution than trying to clone interfaces because cloning interfaces can’t work when an implementation owns noncopyable resources. I also don’t agree a bug was introduced in the commit noted #16963 (comment) or see how #16955 is buggy or would be a bad workaround if documented appropriately.
Anyway, these are just opinions I’m giving since I was tagged earlier #16963 (comment). I don’t feel strongly, and trust @promag’s judgement on what solution to use since he’s taking the initiative here.
or see how #16955 is buggy or would be a bad workaround if documented appropriately.
It’s a workaround due to bad usage of std::unique_ptr and boost::signals2. I think a fix is preferable.
I also don’t agree a bug was introduced in the commit noted
What you mean? can you reproduce #16937 before 1106a6f?
I haven’t seen any response to this suggestion, so I pushed a quick untested implementation
Sweet! 5d7a233193ad2749d326607eb0e995696c8965c8 LGTM, picked and reworded.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
The slot BitcoinGUI::addWallet can be invoked twice for the same
WalletModel due to a concurrent wallet being loaded after the first `connect()`:
```cpp
connect(wallet_controller, &WalletController::walletAdded, this, &BitcoinGUI::addWallet);
connect(wallet_controller, &WalletController::walletRemoved, this, &BitcoinGUI::removeWallet);
for (WalletModel* wallet_model : m_wallet_controller->getOpenWallets()) {
addWallet(wallet_model);
```Second commit 81ea66c30e2953dee24d5b127c28daa0d9452a28 might need more code comments and a better commit description to be more easily reviewable.
http://www.erisian.com.au/bitcoin-core-dev/log-2019-11-02.html#l-128
[11:33:40] wumpus: maybe see #16963? [11:36:06] promag: I honestly don’t see myself qualified to review that [11:36:35] the inter-thread and notification logic around loading wallets is somewhat of a mystery to me
The main thing this change is doing is deleting the boost::signals2::signal<CClientUIInterface::LoadWalletSig> LoadWallet boost signal and replacing it with a std::list<LoadWalletFn> list of callback functions. In general a boost signal can be seen as equivalent to a list of callbacks. Connecting a signal is the same as adding a new callback to the list, disconnecting the signal is the same as dropping a callback from the list, and invoking the signal is the same as iterating over the list and invoking each callback.
The reason for making this change is that the load wallet callback function takes a unique_ptr<interfaces::Wallet> argument that is awkward to share. Problems can happen if the first callback uses std::move on the unique_ptr and later callbacks just see a null pointer. Using std::list instead of boost::signal works better because wallet load code can just pass a different unique_ptr to each callback, and not have to worry about moving or sharing a single value.
Another thing that might look complicated here and could be better documented is the new MakeHandler function:
0//! Return handler wrapping a cleanup function.
1std::unique_ptr<Handler> MakeHandler(std::function<void()> cleanup);
This just takes a std::function<void()> cleanup function, and returns an object which invokes the cleanup function when the object is destroyed. It’s used to erase entries from the std::list<LoadWalletFn> list when the GUI no longer wants to receive callbacks.
This just takes a
std::function<void()>cleanup function, and returns an object which invokes the cleanup function when the object is destroyed. It’s used to erase entries from thestd::list<LoadWalletFn>list when the GUI no longer wants to receive callbacks.
And for that reason a std::list is used - iterators aren’t invalidated if other elements are added/removed.
tested ACK 81ea66c30e2953dee24d5b127c28daa0d9452a28
I was able to reproduce the error using the script from #16937 on master but not with this fix.
code review ACK 6d6a7a8403ae923f189812edebdd95761de0e7f2
I was not able to reproduce the duplicate wallet showing up after ~10 tries on master.
49@@ -48,6 +50,7 @@ bool HasWallets();
50 std::vector<std::shared_ptr<CWallet>> GetWallets();
51 std::shared_ptr<CWallet> GetWallet(const std::string& name);
52 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::vector<std::string>& warnings);
53+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);
HandleLoadWallet arg could be an r-value reference, on account of the std::move calling pattern.
35@@ -36,7 +36,7 @@ class WalletFrame : public QFrame
36
37 void setClientModel(ClientModel *clientModel);
38
39- void addWallet(WalletModel *walletModel);
40+ bool addWallet(WalletModel *walletModel);
bool ready() method that simply checks the conditions rather than arbitrarily booling non-bool methods like this. I think it’s unnecessarily confusing.
6d6a7a8403ae923f189812edebdd95761de0e7f2 @kallewoof thanks for reviewing.
arbitrarily booling non-bool methods like this.
What you mean “like this”? I could refactor by adding bool WalletFrame::hasWallet(WalletModel*) const but IMO this is also fine, for instance, std::set::insert also returns whether the item was added.
What it could have is the NODISCARD attribute, but ATM it’s not used in src/qt.
ready as you suggest.
Yeah, I know this is done elsewhere (even in std), but I don’t think it’s a good idea, personally. It often results in having to look up what exactly is being returned, made even worse with things like bool CreateThing(...). (Looking at the signature unless Thing is a bool type, the return value is most likely the result of the create operation, but seeing it in the wild as auto foo = CreateThing(...) you won’t know if foo is a bool or a Thing without checking.)
Agree that failing silently is a bad idea. Raising sounds good to me, unless you really like the bool approach, in which case keep it as is.