wallet: Fix unique_ptr usage in boost::signals2 #16963

promag · 2019-09-25T10:17:24Z

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.

promag · 2019-09-25T10:17:48Z

@ryanofsky maybe you have a better approach?

ryanofsky · 2019-09-25T11:28:53Z

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:

chain.loadWallet(interfaces::MakeWallet(walletInstance));

with

for (auto& load_wallet : load_wallet_fns) {
    load_wallet(interfaces::MakeWallet(walletInstance));
}

But I'm ok with the nullptr fix or the clone fix or however you want to proceed here.

promag · 2019-09-25T11:48:36Z

IMO it's incorrect because it doesn't remove the invalid std::move.

promag · 2019-09-25T11:50:20Z

which I would expect if m_node.getWallets() is called later

There's nothing guaranteeing it (I think?)

promag · 2019-09-25T19:39:12Z

For reference, the bug was introduced in 1106a6f #15288 - backport not required.

promag · 2019-09-25T19:41:11Z

Related discussion #15288 (comment).

fanquake · 2019-09-26T12:21:51Z

I started playing around with this change (7162be2). 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:

(lldb) run
Process 20318 launched: '/Users/michael/github/bitcoin/src/qt/bitcoin-qt' (x86_64)
2019-09-26 20:14:33.701294+0800 bitcoin-qt[20318:1442147] MessageTracer: Falling back to default whitelist
Process 20318 stopped
* thread #1, name = 'bitcoin-main', queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x10)
    frame #0: 0x000000010009182e bitcoin-qt`GUIUtil::TableViewLastColumnResizingFixer::disconnectViewHeadersSignals(this=0x0000000000000000) at guiutil.cpp:445:16
   442 	// We need to disconnect these while handling the resize events, otherwise we can enter infinite loops.
   443 	void TableViewLastColumnResizingFixer::disconnectViewHeadersSignals()
   444 	{
-> 445 	    disconnect(tableView->horizontalHeader(), &QHeaderView::sectionResized, this, &TableViewLastColumnResizingFixer::on_sectionResized);
   446 	    disconnect(tableView->horizontalHeader(), &QHeaderView::geometriesChanged, this, &TableViewLastColumnResizingFixer::on_geometriesChanged);
   447 	}
   448 	
Target 0: (bitcoin-qt) stopped.

Ran out of time tonight, so will keep looking tomorrow.

src/interfaces/wallet.h

promag · 2019-09-26T13:06:49Z

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:

bitcoin/src/qt/bitcoingui.cpp

Lines 623 to 627 in 6288f15

    
           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);

and if the dynamic loading happens after L623 and before L627.

promag · 2019-09-26T20:53:25Z

Multiple times I've seen bitcoin-qt crash during load with:

@fanquake that happens due to loadwallet? Or have you added other calls to the test script?

fanquake · 2019-09-27T04:23:39Z

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 4066030 so far.

promag · 2019-09-27T06:53:47Z

Going to rebase --autosquash.

ryanofsky · 2019-09-27T11:48:15Z

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 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.

laanwj · 2019-09-27T12:34:47Z

I think calling a refactor a fix is strange (a refactor ,by definition, doesn't change behavior): what is this, a refactor or a bug fix?

promag · 2019-09-27T12:57:31Z

@ryanofsky sorry for not replying, I'll take a look.

@laanwj yeah in that case a fix.

promag · 2019-10-01T15:13:28Z

@ryanofsky

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! 5d7a233 LGTM, picked and reworded.

promag · 2019-10-02T07:04:11Z

Fixed disable wallet builds and squashed.

ryanofsky

utACK fce3b88, but I wrote some code in this, so discount my review a bit.

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); ```

promag · 2019-10-26T13:57:33Z

@ryanofsky fixed, thanks!

ryanofsky

Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to #17070

ryanofsky · 2019-11-04T15:46:47Z

Second commit 81ea66c 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:

//! Return handler wrapping a cleanup function.
std::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.

promag · 2019-11-04T16:34:46Z

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.

And for that reason a std::list is used - iterators aren't invalidated if other elements are added/removed.

fjahr · 2019-12-20T16:05:33Z

tested ACK 81ea66c

I was able to reproduce the error using the script from #16937 on master but not with this fix.

code review ACK 6d6a7a8

I was not able to reproduce the duplicate wallet showing up after ~10 tries on master.

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

Github-Pull: bitcoin#16963 Rebased-From: 6d6a7a8 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); ```

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

Github-Pull: bitcoin#16963 Rebased-From: 6d6a7a8 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); ```

Empact · 2019-12-27T19:39:15Z

src/wallet/wallet.h

@@ -48,6 +50,7 @@ bool HasWallets();
 std::vector<std::shared_ptr<CWallet>> GetWallets();
 std::shared_ptr<CWallet> GetWallet(const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, std::string& error, std::vector<std::string>& warnings);
+std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet);


nit: this HandleLoadWallet arg could be an r-value reference, on account of the std::move calling pattern.

kallewoof

Code review ACK 6d6a7a8

kallewoof · 2020-01-04T23:39:45Z

src/qt/walletframe.h

@@ -36,7 +36,7 @@ class WalletFrame : public QFrame

    void setClientModel(ClientModel *clientModel);

-    void addWallet(WalletModel *walletModel);
+    bool addWallet(WalletModel *walletModel);


I would prefer a bool ready() method that simply checks the conditions rather than arbitrarily booling non-bool methods like this. I think it's unnecessarily confusing.

6d6a7a8

@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.

IMO it's an anti-pattern to silently fail when an operation is attempted, returning a success bool instead makes sense to me in that case, or alternatively raise on failure and use 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.

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: 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 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to #17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

laanwj · 2020-01-08T15:48:22Z

Code review ACK 6d6a7a8.

@ryanofsky Thanks for the explanation that was very helpful

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: 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 bitcoin#16937 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to bitcoin#17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

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); ``` Github-Pull: bitcoin#16963 Rebased-From: 6d6a7a8

cd67b1d Use correct C++11 header for std::swap() (Hennadii Stepanov) b8101fb Fix comparison function signature (Hennadii Stepanov) eac4907 Don't allow implementers to think ScriptHash(Witness*()) results in nesting computation (Gregory Sanders) e2c45d8 IsUsedDestination shouldn't use key id as script id for ScriptHash (Gregory Sanders) a5489c9 IsUsedDestination should count any known single-key address (Gregory Sanders) 88729d8 Fix issue with conflicted mempool tx in listsinceblock (Adam Jonas) eafcea7 gui: Fix duplicate wallet showing up (João Barbosa) 7e66d04 Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) 179d55f zmq: Fix due to invalid argument and multiple notifiers (João Barbosa) Pull request description: Backports - #16963 - #17445 - #17258 - #17621 - #17924 - #17634 ACKs for top commit: laanwj: ACK cd67b1d, checked that I got more or less the same result (including conflict resolution) backporting these commits Tree-SHA512: 645786267cfb10a01a56f7cfd91ddead5f1475df5714595ae480237e04d40c5cfb7460b40532279cacd83e4b775a4ace68a258ec2184b8ad0e997a690a9245e5

Summary: * Drop signal CClientUIInterface::LoadWallet * gui: Fix duplicate wallet showing up 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); ``` This is a backport of Core [[bitcoin/bitcoin#16963 | PR16963]] Because `WalletFrame::addWallet` already returns a bool, that part of the patch wasn't necessary. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7699

6d6a7a8 gui: Fix duplicate wallet showing up (João Barbosa) 81ea66c Drop signal CClientUIInterface::LoadWallet (Russell Yanofsky) Pull request description: 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 bitcoin#16937 ACKs for top commit: fjahr: code review ACK 6d6a7a8 ryanofsky: Code review ACK 6d6a7a8. No changes since last ACK other than rebase due to bitcoin#17070 kallewoof: Code review ACK 6d6a7a8 Tree-SHA512: 7f0658c9011f81dfa176a094c2263448ee1d14fda7dc94e8b55ee9c8b81538bd2d1e4bf8a8dbfcd029ebfc9feb6d3cda9dee3f911122df0a4b1e0ca75f653ba4

Summary: This is a backport of Core [[bitcoin/bitcoin#15464 | PR15464]] `addWallet` is not modified because the return value is added back and used in [[bitcoin/bitcoin#16963 | PR16963]] which was backported in D7699. Test Plan: `ninja && src/qt/bitcoin-qt` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D8363

fanquake added the Refactoring label Sep 25, 2019

maflcko added Wallet and removed Refactoring labels Sep 25, 2019

promag mentioned this pull request Sep 25, 2019

gui: only call getOrCreateWallet if wallet interface pointer not null #16955

Closed

fanquake reviewed Sep 26, 2019

View reviewed changes

src/interfaces/wallet.h Outdated Show resolved Hide resolved

promag force-pushed the 2019-09-fix-loadwallet-signal-uniqueptr branch from 4066030 to 52b9d00 Compare September 27, 2019 06:55

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 27, 2019

suggestion bitcoin#16963 (comment)

0d76b8e

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 27, 2019

suggestion bitcoin#16963 (comment)

5d7a233

maflcko changed the title ~~refactor: Fix unique_ptr usage in boost::signals2~~ wallet: Fix unique_ptr usage in boost::signals2 Sep 27, 2019

laanwj added the Bug label Sep 30, 2019

promag force-pushed the 2019-09-fix-loadwallet-signal-uniqueptr branch 3 times, most recently from 40b4d25 to fce3b88 Compare October 2, 2019 07:02

ryanofsky approved these changes Oct 2, 2019

View reviewed changes

promag force-pushed the 2019-09-fix-loadwallet-signal-uniqueptr branch from e956882 to 6d6a7a8 Compare October 26, 2019 13:57

ryanofsky approved these changes Oct 28, 2019

View reviewed changes

promag pushed a commit to promag/bitcoin that referenced this pull request Dec 23, 2019

Drop signal CClientUIInterface::LoadWallet

eb3c824

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

promag mentioned this pull request Dec 23, 2019

0.19: Backports #17792

Merged

promag pushed a commit to promag/bitcoin that referenced this pull request Dec 23, 2019

Drop signal CClientUIInterface::LoadWallet

7f9649e

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

Empact reviewed Dec 27, 2019

View reviewed changes

kallewoof reviewed Jan 4, 2020

View reviewed changes

laanwj merged commit 6d6a7a8 into bitcoin:master Jan 8, 2020

promag deleted the 2019-09-fix-loadwallet-signal-uniqueptr branch January 8, 2020 15:01

hebasto mentioned this pull request Jan 8, 2020

gui: Bilingual GUI error messages #16224

Merged

promag pushed a commit to promag/bitcoin that referenced this pull request Jan 14, 2020

Drop signal CClientUIInterface::LoadWallet

7e66d04

Github-Pull: bitcoin#16963 Rebased-From: 81ea66c

MarkLTZ added a commit to litecoinz-core/litecoinz that referenced this pull request Feb 3, 2020

[wallet] Fix unique_ptr usage in boost::signals2 (bitcoin#16963)

7433507

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky mentioned this pull request Jun 4, 2020

Multiprocess bitcoin #10102

Draft

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

wallet: Fix unique_ptr usage in boost::signals2 #16963

wallet: Fix unique_ptr usage in boost::signals2 #16963

Uh oh!

Conversation

promag commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 25, 2019

Uh oh!

ryanofsky commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 25, 2019

Uh oh!

promag commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 25, 2019

Uh oh!

promag commented Sep 25, 2019

Uh oh!

fanquake commented Sep 26, 2019

Uh oh!

Uh oh!

promag commented Sep 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 26, 2019

Uh oh!

fanquake commented Sep 27, 2019

Uh oh!

promag commented Sep 27, 2019

Uh oh!

ryanofsky commented Sep 27, 2019

Uh oh!

laanwj commented Sep 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag commented Sep 27, 2019

Uh oh!

promag commented Oct 1, 2019

Uh oh!

promag commented Oct 2, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 26, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Nov 4, 2019

Uh oh!

promag commented Nov 4, 2019

Uh oh!

fjahr commented Dec 20, 2019

Uh oh!

Empact Dec 27, 2019

Choose a reason for hiding this comment

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 4, 2020

Choose a reason for hiding this comment

Uh oh!

promag Jan 4, 2020

Choose a reason for hiding this comment

Uh oh!

Empact Jan 4, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 5, 2020

Choose a reason for hiding this comment

Uh oh!

laanwj commented Jan 8, 2020

Uh oh!

Uh oh!

promag commented Sep 25, 2019 •

edited

Loading

ryanofsky commented Sep 25, 2019 •

edited

Loading

promag commented Sep 25, 2019 •

edited

Loading

promag commented Sep 26, 2019 •

edited

Loading

laanwj commented Sep 27, 2019 •

edited

Loading