wallet: early listener set

[ph4r05]

API changes to enable passphrase entry on host via API (e.g., for GUI) - callback triggers request for passphrase if required. If callback is set before opening / creating wallet from a device it can set the passphrase to the device.

I had to add another callback method to set wallet obj after it is created as callbacks usually need to hold it.

@moneromooo-monero what do you think about this approach?

[moneromooo-monero]

The wallet api is not really something I pay much attention to, ask the GUI people. I think currently it's dsc mostly. I'm OK with the patch myself, and if nobody from the GUI comments it'll go in.

[ph4r05]

After giving it a second thought I think this is an easy way to handle device init stuff (like passphrase entry on host) without any massive refactoring. @MaxXor @xmrdsc ? Do you think it can be approved? :)

[ph4r05]

The GUI PR which makes use of this PR is here: monero-project/monero-gui#2037

[sanderfoobar]

Looks fine to me - does not seem to influence existing behaviour from the perspective of the GUI.

[MaxXor]

Looks good to me as well!

[vtnerd]

How is the lifetime of the wallet pointer handled? This should be a std::shared_ptr for sanity purposes; any of these calls that throw is a potential memory leak.

[ph4r05]

How is the lifetime of the wallet pointer handled? This should be a std::shared_ptr for sanity purposes; any of these calls that throw is a potential memory leak.

@vtnerd TBH I don't see an easy way a std::shared_ptr could be used here. E.g., the Wallet *WalletManagerImpl::createWalletFromDevice() returns the raw pointer here so passing std::shared_ptr to the WalletListener::onSetWallet() won't work without API changes. There is no std::shared_ptr used in the Wallet API so using it in the listener would probably need major refactoring.

Moreover, callbacks usually don't hold strong references to the owners to avoid circular references, right? I thought that the callback doesn't use the wallet pointer unless it is invoked by the wallet itself - pointer should be valid in that case.

Currently, I see only one alternative - call WalletListener::onSetWallet(nullptr) when wallet is deallocated. Maybe call it in the ~Wallet2CallbackImpl() ? Or should it be done rather in WalletManagerImpl::closeWallet ?

Or how do you think this could work? I am out of easy ideas right now.

[ph4r05]

@vtnerd ping

[ph4r05]

As the changes look good for GUI guys, can we maybe approve pls ? :)

[sanderfoobar]

I said "Looks fine to me" in the sense that you found a solution that does not require changes to our existing logic in the GUI, which is nice. I cannot really vouch for the quality of C++ itself - others are more experienced :)

[vtnerd]

@vtnerd TBH I don't see an easy way a std::shared_ptr could be used here. E.g., the Wallet *WalletManagerImpl::createWalletFromDevice() returns the raw pointer here so passing std::shared_ptr to the WalletListener::onSetWallet() won't work without API changes. There is no std::shared_ptr used in the Wallet API so using it in the listener would probably need major refactoring.

Improving the wallet pointer lifetimes are likely out-of-scope for this patch and should be slowly updated to improve lifetime guarantees of pointers. Using smart pointers for this listener parameter are possible I think.

The GUI PR creates the listener on the stack, which gets destroyed when the function leaves scope. Doesn't that cause a dangling reference? It might be working based that block of code if the memory is not re-used.

It looks like you want sole ownership of the listener? The semantics aren't clear at all from this change. Just take the listener by unique_ptr if sole ownership is desired and shared_ptr otherwise.

Moreover, callbacks usually don't hold strong references to the owners to avoid circular references, right? I thought that the callback doesn't use the wallet pointer unless it is invoked by the wallet itself - pointer should be valid in that case.

std::weak_ptr exists for this reason.

Currently, I see only one alternative - call WalletListener::onSetWallet(nullptr) when wallet is deallocated. Maybe call it in the ~Wallet2CallbackImpl() ? Or should it be done rather in WalletManagerImpl::closeWallet ?

[ph4r05]

The GUI PR creates the listener on the stack, which gets destroyed when the function leaves scope. Doesn't that cause a dangling reference? It might be working based that block of code if the memory is not re-used.

I agree that it could be improved, but not by this patch.

The callback is reset to a new valid callback as the wallet is created. Callback needs to exist only while wallet is being created / restored. It is reset after that by the manager.

It looks like you want sole ownership of the listener? The semantics aren't clear at all from this change. Just take the listener by unique_ptr if sole ownership is desired and shared_ptr otherwise.

I do not require sole ownership, that would be unnecessary limitation. Callback could be manager itself.

Listener is already in API as plain pointer, that would break the GUI. Shared_ptr could deallocate pointer which cannot be deallocated because it could be owned by GUI (plain pointer breaks relation).

std::weak_ptr exists for this reason.

Sure, I know. But I have no idea how that helps without refactoring smart pointers for listener.

Could you maybe scratch the idea in pseudocode how that would work?

[vtnerd]

I agree that it could be improved, but not by this patch.

The callback is reset to a new valid callback as the wallet is created. Callback needs to exist only while wallet is being created / restored. It is reset after that by the manager

This will be difficult to maintain - the chunk of code creating a temporary listener has to rely on a sequence of events elsewhere and it won't be obvious to someone modifying that constructor about a dangling reference possibility in some other arbitrary file. In this particular situation the dangling reference modification is probably unlikely to happen, but this type of brittleness is a common way to introduce UB through maintenance.

At a glance it seems like there has to be a way to update the construction sequence going on here - a temporary listener just during construction is odd - but that might require some decent re-arranging.

Sure, I know. But I have no idea how that helps without refactoring smart pointers for listener.

Could you maybe scratch the idea in pseudocode how that would work?

What exactly? How to use weak_ptr or how to use shared_ptr for the listener? I'll assume the latter, weak_ptr doesn't appear to be needed for my simple suggestion.

Use std::shared_ptr for the wallet listener member variable and parameters. Use std::make_shared to create the listener. That's basically it. The GUI is using the listener in one header/class pair right now and a second with the referenced PR. The code changes are minimal, and at least it would be a start at helping managing the myriad pointer lifetimes in that code. The listener in particular is tricky due to the multiple ownership.

[vtnerd]

Listener is already in API as plain pointer, that would break the GUI. Shared_ptr could deallocate pointer which cannot be deallocated because it could be owned by GUI (plain pointer breaks relation).

One thing to make certain - if shared_ptr is being used then no "raw" pointers should be used unless its a "temporary" thing in a parameter but not permanently stored. The shared_ptr keeps the pointer active until all references are released.

[ph4r05]

This will be difficult to maintain - the chunk of code creating a temporary listener has to rely on a sequence of events elsewhere and it won't be obvious to someone modifying that constructor about a dangling reference possibility in some other arbitrary file. In this particular situation the dangling reference modification is probably unlikely to happen, but this type of brittleness is a common way to introduce UB through maintenance.

This PR only adds an option to set a listener during the wallet creation. Previosly, it was allowed only after the creation, but in the same way (raw pointer). So it is basically the same logic. Temporary listener is thing of GUI PR which handles the temporary nature in the GUI Wallet contructor. But thats maybe for a different GUI PR discussion.

Use std::shared_ptr for the wallet listener member variable and parameters. Use std::make_shared to create the listener. That's basically it. The GUI is using the listener in one header/class pair right now and a second with the referenced PR. The code changes are minimal, and at least it would be a start at helping managing the myriad pointer lifetimes in that code. The listener in particular is tricky due to the multiple ownership.

Hmm and that's a problematic point, I think. We would basically force all wallet API users to migrate to smart pointers on listeners with this PR. The smart pointers proposition breaks backward compatibility on the public API which maybe should not happen besides major version change. GUI is one known API user but there may be others (e.g., exchanges?).

Take simplewallet as an example (not API related, just as a design choice). It is itself a listener. By the smart pointers change we would force the simplewallet creator to use smart pointer for storing simplewallet.

Adding smart pointer would break API user logic as they deallocate listeners on their own now (probably). I would like to avoid introducing API-breaking changes, which are probably more dangerous for third parties.

[ph4r05]

At a glance it seems like there has to be a way to update the construction sequence going on here - a temporary listener just during construction is odd - but that might require some decent re-arranging.

The listener is set to the wallet so it could be both permanent and temporary. Temporary justifies its purpose by allowing to catch passphrase request during wallet creation with existing API without any major overhaul needed.

Permanent could be set by the API user. In fact this PR does not make a difference whether it is temporary or permanent. GUI PR uses it as temporary because their own code

// src/libwalletqt/WalletManager.cpp
m_currentWallet = new Wallet(w); 

takes ownership of Monero::Wallet * w and sets own listener in the constructor (after wallet is opened/created). So in my opinion, there is no hidden cross-API dependency with respect to listeners. To be sure, I've just updated GUI PR to reset callback before the Wallet(w). Now it is more explicit.

I mean the way this PR works is in line with existing API which also takes listener as raw pointer. So there are no logic changes in that. Adding smart pointers to one place and breaking API with it is IMO dangerous.

[ph4r05]

@vtnerd do you think that the newest changes I've made address objections you've had?

[ph4r05]

@moneromooo-monero what do you think?

[vtnerd]

@vtnerd do you think that the newest changes I've made address objections you've had?

I assume you meant the setListener(nullptr) change. I was going to suggest this after initially reading the last couple of responses by you.

Use std::shared_ptr for the wallet listener member variable and parameters. Use std::make_shared to create the listener. That's basically it. The GUI is using the listener in one header/class pair right now and a second with the referenced PR. The code changes are minimal, and at least it would be a start at helping managing the myriad pointer lifetimes in that code. The listener in particular is tricky due to the multiple ownership.

Hmm and that's a problematic point, I think. We would basically force all wallet API users to migrate to smart pointers on listeners with this PR. The smart pointers proposition breaks backward compatibility on the public API which maybe should not happen besides major version change. GUI is one known API user but there may be others (e.g., exchanges?).

The changes should be minimal for upstream, but it is difficult to say with certainty.

Take simplewallet as an example (not API related, just as a design choice). It is itself a listener. By the smart pointers change we would force the simplewallet creator to use smart pointer for storing simplewallet.

I don't see where simplewallet is using the listener API or the wallet "API" at all. Simplewallet is using wallet2 directly AFAIK.

At a glance it seems like there has to be a way to update the construction sequence going on here - a temporary listener just during construction is odd - but that might require some decent re-arranging.

The listener is set to the wallet so it could be both permanent and temporary. Temporary justifies its purpose by allowing to catch passphrase request during wallet creation with existing API without any major overhaul needed.

Permanent could be set by the API user. In fact this PR does not make a difference whether it is temporary or permanent. GUI PR uses it as temporary because their own code

The primary frustration as a reviewer was having to dig through the implementation to try and determine how the lifetime of the pointers are supposed to work (who was supposed to be responsible for cleanup, etc.). It seems that the pointer lifetime is managed by the client who must destroy the api::Wallet or call setListener before destroying the listener.

// src/libwalletqt/WalletManager.cpp
m_currentWallet = new Wallet(w);
takes ownership of Monero::Wallet * w and sets own listener in the constructor (after wallet is opened/created). So in my opinion, there is no hidden cross-API dependency with respect to listeners. To be sure, I've just updated GUI PR to reset callback before the Wallet(w). Now it is more explicit.

Taking ownership of w makes it obvious m_currentWallet now has to manage the listeners? I guess? I'm not sure that I would assume that from the chunk of code.

I mean the way this PR works is in line with existing API which also takes listener as raw pointer. So there are no logic changes in that. Adding smart pointers to one place and breaking API with it is IMO dangerous.

Part of my feedback was giving recommendations for future changes/directions, not necessarily immediately requesting changes. Using smart ptrs generally helps with guidance on ownership in the absence of explicit documentation.

[ph4r05]

I assume you meant the setListener(nullptr) change. I was going to suggest this after initially reading the last couple of responses by you.

Correct, this one. I hope it makes it more clear and safer.

The primary frustration as a reviewer was having to dig through the implementation to try and determine how the lifetime of the pointers are supposed to work (who was supposed to be responsible for cleanup, etc.). It seems that the pointer lifetime is managed by the client who must destroy the api::Wallet or call setListener before destroying the listener.

Taking ownership of w makes it obvious m_currentWallet now has to manage the listeners? I guess? I'm not sure that I would assume that from the chunk of code.

I had to dig it too, it seems that src/libwalletqt/Wallet.cpp manages the listener by itself as it allocates and deallocates it (constructor/destructor). So I think the API is designed in a way that client manages listener pointer lifetime and it works as you said.

Part of my feedback was giving recommendations for future changes/directions, not necessarily immediately requesting changes. Using smart ptrs generally helps with guidance on ownership in the absence of explicit documentation.

Thanks for the feedback anyway! I like using smart pointers and taking inspiration from other developers. This definitely could be place to refactor next time.

[ph4r05]

(rebased)

[ph4r05]

I've added simple user action request tracking which is helpful for GUI - to let user know that action on the device is required. GUI needs also callback on button pressed event e.g., to hide processing splash (as user action was already performed).

@moneromooo-monero do you pls think this PR could go in? Thanks! :)

[fluffypony]

Reviewed