Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

wallet: Add close_wallet RPC #4007

Merged
merged 1 commit into from Sep 10, 2018

Conversation

Projects
None yet
6 participants
@glv2
Copy link
Contributor

commented Jun 16, 2018

For issue #3227

@@ -2287,6 +2287,12 @@ namespace tools
//------------------------------------------------------------------------------------------------------------------------------
bool wallet_rpc_server::on_create_wallet(const wallet_rpc::COMMAND_RPC_CREATE_WALLET::request& req, wallet_rpc::COMMAND_RPC_CREATE_WALLET::response& res, epee::json_rpc::error& er)
{
if (m_wallet)
{
er.code = WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR;

This comment has been minimized.

Copy link
@moneromooo-monero

moneromooo-monero Jun 16, 2018

Contributor

Add a new error since you know what the error is here.

This comment has been minimized.

Copy link
@glv2

glv2 Jun 16, 2018

Author Contributor

Ok.

@glv2 glv2 force-pushed the glv2:close-wallet-rpc branch from b5a6851 to 2e978ba Jun 16, 2018

bool wallet_rpc_server::on_close_wallet(const wallet_rpc::COMMAND_RPC_CLOSE_WALLET::request& req, wallet_rpc::COMMAND_RPC_CLOSE_WALLET::response& res, epee::json_rpc::error& er)
{
if (!m_wallet) return not_open(er);
if (m_wallet->restricted())

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

I don't think this method needs to be restricted.

This comment has been minimized.

Copy link
@glv2

glv2 Jun 16, 2018

Author Contributor

I didn't allow on_close_wallet in restricted to match the behaviour of on_stop_wallet.

Do we want to allow a client connected to a restricted RPC server to open and close wallets at will, or not?

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

Restricted is not to stop users, it's to limit potential privacy leaking information IIRC. You'll notice it's only being used on things like transfers. Not open.


try
{
m_wallet->store();

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

I don't think you need a store.

This comment has been minimized.

Copy link
@glv2

glv2 Jun 16, 2018

Author Contributor

on_stop_wallet calls m_wallet->store(), so I did the same thing here.

Are we sure that the wallet cache file would be up to date if we deleted m_wallet without doing a store() first?

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

There are multiple places where it is not called though. The cache would be updated as long as the server refresh loop is running, which it would be.

This comment has been minimized.

Copy link
@artyomsol

artyomsol Jun 17, 2018

Contributor

@jtgrassie @glv2

The cache would be updated as long as the server refresh loop is running, which it would be.

The thing I've faced with v.0.12.2.0 is that opening the other wallet or stopping the wallet with the stop_wallet method results to the tx_keys collected during transfer creations being lost. But if I do call the store method before stopping or switching the wallet the private keys of outgoing transfers persisted as expected.
IHMO, store() need to be done within close_wallet method.

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 17, 2018

Contributor

Fair enough.

m_wallet = wal.release();
return true;
}
//------------------------------------------------------------------------------------------------------------------------------
bool wallet_rpc_server::on_open_wallet(const wallet_rpc::COMMAND_RPC_OPEN_WALLET::request& req, wallet_rpc::COMMAND_RPC_OPEN_WALLET::response& res, epee::json_rpc::error& er)
{
if (m_wallet)

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

There is no harm leaving this method alone. Users may have code already using open_wallet to open a new wallet.

@@ -2424,12 +2434,34 @@ namespace tools
er.message = "Failed to open wallet";
return false;
}
if (m_wallet)

This comment has been minimized.

Copy link
@jtgrassie

jtgrassie Jun 16, 2018

Contributor

As per above.

@hyc

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

While we're mucking about in here, it's probably time to take a step back and ask what we're trying to accomplish. Originally there was monero-wallet-cli which had the RPC listener built in. It was clearly a single-user/single-wallet system. Now monero-wallet-rpc has been split off into its own program, and we needed create_wallet so that a GUI wrapper could be written for it that has all the functionality of monero-wallet-cli without having to resort to awkward cli invocations.

It sounds like people are trying to use monero-wallet-rpc now as a multi-user wallet server. It most definitely is not suited for that purpose in its current form. Is that where we want to take it? It will need actual session management before that can happen.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

@hyc I think the problem is we cannot anticipate all peoples use cases and the option to close wallet has come up a few times now on various channels and this does appear an obvious missing method to me.

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 16, 2018

I agree that in its current state monero-wallet-rpc shouldn't be used as a wallet server for multiple users. For the moment the close_wallet RPC will be useful only for the "single user with multiple wallets" use case.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

Is the method really needed though? A single user would just call open_wallet, do stuff, then open_wallet with another wallet file. This already works. No need for closing in this single user use case.

@artyomsol

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

Wallet closing required in case of a server-to-server integration with low frequency wallet interactions. Lets say web master wants to automate the donations collection and tend to check the wallet state couple times per day or on-demand.
Wallet file must be kept closed (unloaded from memory) on a RPC server between sessions for the security reasons, I guess. While RPC server process must be up and running all the time.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

@artyomsol

Wallet file must be kept closed (unloaded from memory) on a RPC server between sessions for the security reasons, I guess.

Please explain security concern.

@artyomsol

This comment has been minimized.

Copy link
Contributor

commented Jun 16, 2018

@jtgrassie

Please explain security concern.

By reducing the time frame of wallet being loaded to the memory and API being able to process security sensitive requests the one can reduce the attack surface.

@glv2 glv2 force-pushed the glv2:close-wallet-rpc branch from 2e978ba to c3ed2e5 Jun 17, 2018

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

@artyomsol surely if you are that concerned of a security issue with a loaded wallet you would also need to wipe the memory as simply deleting the object isn't guaranteed to immediately clear it from memory. TBH, I'm coming round to @hyc's point now that we need to take a step back as monero-wallet-rpc was not designed for the use cases people are looking to use it for it seems.

@artyomsol

This comment has been minimized.

Copy link
Contributor

commented Jun 17, 2018

@jtgrassie

monero-wallet-rpc was not designed for the use cases people are looking to use it

Its not about multi-user wallet access use case. Its all about personal use of RPC API for unattended run. Is there any alternative wallet API to integrate with for 24/7 up time ?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

Is that still being worked on ?

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

I think this PR is ok in its current state.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

#4007 (review) and #4007 (comment) not addressed. The latter of which may break functionality of 3rd party stuff as it's changing existing functionality.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

Looks alright, though that'll force existing RPC users to call close_wallet all the time now. Not sure that's a big problem.

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

@jtgrassie, according to @artyomsol's comment, a store() is required on the current wallet before opening another one, or some data may be lost. Therefore forcing the user to close the current wallet (and storing it) before opening another one is necessary.
Moreover, as currently in the code store() is a restricted method, close_wallet() should be too.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

Might be a good idea to add a "save" bool to the request. Easy to do, can default to true, and avoids having to write if it's unwanted.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@glv2 I'm talking about the store comments. I'm talking about the changes to on_open_wallet which is now preventing opening a wallet without first closing the existing. This may break anything currently relying on the existing behavior.

@hyc

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

I suggest adding a wallet->dirty flag, set it true whenever anything is modified, and auto-store if dirty when closing. I don't think we want to change to requiring an explicit close before opening a new one, that's just a wasted roundtrip.

Add two dirty flags - one for the keys file (settings) and a separate one for the cache file.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

There's gonna be a LOT of places where this flag needs setting...

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

We could also consider store() as not being a restricted method, and store and close the current wallet automatically if necessary when calling on_open_wallet(). Is there a case where this would be problematic?

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@glv2 I mentioned in another comment about the restricted functions. It's for preventing privacy related leakage, nothing more. You are thinking "restricted" in terms of functionality which is not what it is for in this context.

open_wallet IMHO should function as it currently does, even if you add a close method. In fact you could just call the close from open.

I particularly like @hyc comment about not storing if it's not required also.

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

@jtgrassie The help of monero-wallet-rpc indicates that the --restricted-rpc option "Restricts to view-only commands", which looks like a functionality restriction and tells nothing about privacy protection.

If this option was only to prevent privacy leaks, why would functions like on_store() and on_stop_wallet() be restricted?

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@glv2 My recollection, was that it was for preventing privacy info calls, but happy to be wrong. If that's the case, the open_wallet command still shouldn't be restricted - opening a wallet is required to use other commands whether view-only or not.

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

I implemented the alternative described in one of my previous comments (#4007 (comment)) in glv2@fd3cc82. Should I replace the current PR by this?

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

So you have made changes in a different branch than this PR was created from?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

I can't recall exactly why I added restricted mode for the wallet, because it makes much less sense than for the daemon, but I suspect it was to allow setting up a wallet RPC without the ability to get your monero stolen (at least without exploit).

Edit: actually, git log sheds light:

commit 24d500c
Author: moneromooo-monero moneromooo-monero@users.noreply.github.com
Date: Sun Jan 11 11:06:35 2015 +0000

Add a --restricted-rpc flag to simplewallet

It restricts RPC to a subset of "view only" commands. Kind of like
a poor man's view key replacement.
@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Jun 29, 2018

glv2@fd3cc82 is just the current PR plus unrestricted wallet closing and automatic wallet closing when opening another wallet, that I squached and rebased on current master to check that it works.

If we decide to do things this way, it might make sense to remove the restriction for on_store() and on_stop_wallet() for consistency...

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@glv2 why not simply add the changes to the branch you PR'ed against? That way it's all tracked.

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2018

@moneromooo-monero when you state "view only" do you mean commands that do not modify the wallet? Or something else?

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

I believe my original intent was to avoid theft. Since we now have view wallets, this is not really needed anymore, and if there's a better "niche" for the restricted set of commands, I'm fine with it changing semantics if people think it's better. I don't have a strong opinion here, but I'd tend towards not allowing open/close (again, no strong opinion).

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

@moneromooo-monero

but I'd tend towards not allowing open/close (again, no strong opinion).

If open/close are restricted then it kind-of defeats the purpose of the restricted mode does it not? E.g. if I start the process in restricted mode, how does one open a wallet.

Given your original intent, I'd suggest either removing restricted altogether (as you say, we have read only wallets for this) or only have restricted on methods that can spend / alter the wallet (essentially yielding view only access).

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

You only need that call if you start with --wallet-dir IIRC.

@hyc

This comment has been minimized.

Copy link
Contributor

commented Jun 30, 2018

Correct. If you start with --wallet-file this is irrelevant, and you get the original behavior.
Still sounds to me like close_wallet is not a useful RPC to add.

@moneromooo-monero

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

Is there anything pending here ?

@jtgrassie

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

I'm not personally in favor of changing existing functionality to add a new method. (e.g. create and open has changed). But also I'm not hugely opinionated on the matter of adding a close method (as it seems an obvious missing feature).

@artyomsol

This comment has been minimized.

Copy link
Contributor

commented Aug 21, 2018

@glv2 Would you please revert the changes made to the open and to the create methods, just to make it possible to merge your PR to the next release. Thanks in advance!

@glv2 glv2 force-pushed the glv2:close-wallet-rpc branch from c3ed2e5 to f809528 Aug 22, 2018

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2018

I updated the PR to allow users to create/open a new wallet without closing the current one first. The state of the current wallet is saved automatically if necessary in order not to lose data.

@artyomsol

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

@glv2 I am sorry to say, but You did change the behavior of create/open again.
In case of any failure the original implementation keeps current wallet open. Your code will store and forget current wallet even if the main method task will fail for some reason.
Am I right ?

@jtgrassie jtgrassie referenced this pull request Sep 4, 2018

Closed

Wallet RPC developer guide - Full refresh #854

72 of 72 tasks complete
@artyomsol
Copy link
Contributor

left a comment

@glv2 could you please fix it to get PR merged to the master ASAP.

handle_rpc_exception(std::current_exception(), er, WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR);
return false;
}
delete m_wallet;

This comment has been minimized.

Copy link
@artyomsol

artyomsol Sep 4, 2018

Contributor

This will leave wallet rpc daemon without any wallet open if it face Failed to generate wallet error for example.

handle_rpc_exception(std::current_exception(), er, WALLET_RPC_ERROR_CODE_UNKNOWN_ERROR);
return false;
}
delete m_wallet;

This comment has been minimized.

Copy link
@artyomsol

artyomsol Sep 4, 2018

Contributor

The same as above ... Open must close former wallet if and only if a new wallet file open succeed

wallet rpc: Add close_wallet RPC
And close the current wallet automatically if necessary when opening another
wallet.

@glv2 glv2 force-pushed the glv2:close-wallet-rpc branch from f809528 to 54b859b Sep 5, 2018

@glv2

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2018

Pushed fixes to close/save the current wallet only after the new one is successfully loaded and to solve conflicts with the master branch.

@luigi1111 luigi1111 merged commit 54b859b into monero-project:master Sep 10, 2018

0 of 7 checks passed

buildbot/monero-linux-armv7 Build done.
Details
buildbot/monero-linux-armv8 Build done.
Details
buildbot/monero-static-osx-10.11 Build done.
Details
buildbot/monero-static-osx-10.12 Build done.
Details
buildbot/monero-static-osx-10.13 Build done.
Details
buildbot/monero-static-ubuntu-amd64 Build done.
Details
buildbot/monero-static-win64 Build done.
Details

luigi1111 added a commit that referenced this pull request Sep 10, 2018

Merge pull request #4007
54b859b wallet rpc: Add close_wallet RPC (glv2)

@glv2 glv2 deleted the glv2:close-wallet-rpc branch Sep 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.