wallet2: No longer create '.address.txt' files for non-testnet wallets #3315

leonklingele · 2018-02-25T17:03:59Z

Previously, a file containing the unencrypted Monero address was
created in the wallet's directory. This file is only used for testing
purposes and might pose as a privacy risk.

iDunk5400 · 2018-02-25T19:48:01Z

NACK.

This file is only used for testing purposes and might pose as a privacy risk.

Are you one of the Cryptonote and/or Bytecoin authors who put that there for testing purposes and then later forgot to take it out? If not, why would you want to remove such useful functionality from the wallet and prevent anyone from being able to quickly associate wallet files with their respective addresses? If someone can see the contents of that file on your computer, you've got bigger problems than your public address getting exposed.

leonklingele · 2018-02-25T22:19:20Z

How is this feature useful to you @iDunk5400? I was under the impression that the file was only used for unit tests inside Monero:

monero/tests/libwallet_api_tests/scripts/send_funds.sh

Line 5 in f195a44

local dest=$(cat "$2.address.txt")

If not completely getting rid of the file, we should at least offer a way to opt-out / explicitly opt-in by providing some CLI flag.

fyi @moneromooo-monero as we talked on IRC.

iDunk5400 · 2018-02-25T23:39:35Z

It is very useful to me as I can see which address a .keys file uses without opening it, if I want to quickly give a particular address to a sender. I don't have to open and log in to (possibly) many wallets to find the address I'm looking for. That's, basically, the purpose of having the *.address.txt file.

arnuschky · 2018-02-26T08:36:17Z

I disagree agree with @iDunk5400, even though I use it the same way.

I think your public address being visible is definitely a privacy risk. Yes, you might have a problem if someone can access your computer, but this is exactly why the wallet is encrypted. With your address being stored along side of it, you'll lose all plausible deniability.

You can still keep a file alongside your wallets that lists the public addresses. As this is a bit inconvenient, how about the following compromise: On wallet creation, we prompt the user whether he wants to write the unencrypted address to a file?

moneromooo-monero · 2018-02-26T09:13:17Z

I think it's mildly good to have this file not written out by default. If people use it, then a flag to enable this would be good. Then those who want it have it, and people who don't know about it don't get their address written out where they might not expect it.

arnuschky · 2018-02-26T09:41:30Z

Well, wallet creation is inherently an interactive process, so why not just prompt the user?

leonklingele · 2018-02-26T10:40:57Z

Added a way to opt-in to continue creating the address file for new wallets.

stoffu · 2018-02-26T11:04:09Z

src/wallet/wallet2.h

@@ -1137,6 +1142,7 @@ namespace tools
    NodeRPCProxy m_node_rpc_proxy;
    std::unordered_set<crypto::hash> m_scanned_pool_txs[2];
    size_t m_subaddress_lookahead_major, m_subaddress_lookahead_minor;
+    bool m_create_address_file;


Because wallet creation is only a one-time operation, the introduction of this member data doesn't make sense to me; it can simply be a local variable.

Also, I'd achieve the same goal without adding yet another command line flag (there are already quite many); the program can simply prompt the user during the wallet creation.

OK, but add a flag to the json fo generate_from_json, and make sure there is no prompt in that case.

I am against adding another interactive step for a user to follow in the wallet creation process. This just makes it more complicated to get started and I reckon a lot of first-time users are not interested in such an address file anyway.

I changed my mind and agree with not doing it interactively.

In that case I'm fine with not doing it at all. New users who aren't expecting it to happen won't be surprised then. Anyone who wants a copy of the address can just use the address command in the wallet and paste it into a file if they want to.

@hyc I agree. Better a little inconvenience than compromising on privacy.

leonklingele · 2018-02-27T08:54:51Z

Cherry-picked @stoffu's commit, ptal

stoffu · 2018-02-28T07:54:54Z

@leonklingele Please squash the second commit because it reverts some changes made in the first one.

Gingeropolous · 2018-03-02T17:34:33Z

I have enjoyed the .address.txt files for the same reason that @iDunk5400 mentioned - being able to get the address associated with a wallet without opening it, but it would be fine if it was a wallet command ... not that there needs to be anymore commands, but whattaya gonna do.

hyc · 2018-03-02T22:53:31Z

It has always been a wallet command.

fluffypony

Reviewed

fluffypony · 2018-03-05T17:13:59Z

@leonklingele please rebase

leonklingele · 2018-03-05T23:42:27Z

Rebased & refreshed commit message.

ghost · 2018-03-06T21:08:45Z

I also prefer to keep the file. I use it similarly to iDunk and Gingeropolous.

m2049r · 2018-03-10T10:27:09Z

my 2 cents:

This breaks the API as it behaves differently than before. I feel strongly that APIs should not be broken unless in case of emergency. Which this is not. Wallets using the API will either break or function differently than before. When breaking the API I think it would be important to give the consumers of the API time to adjust (2 weeks is not enough - 2-3 release cycles is more appropriate). They also need to be informed of such changes (eg. via changelog). Obviously this is nothing specific to this PR.

I see no API code here to create the address.txt at a later time. And none for removing it if the user opted to generate it on wallet creation.

I also feel that that we should not have the API (or anything else in general) behave differently on testnet than on mainnet.

moneromooo-monero · 2018-03-10T10:45:40Z

This patch does not touch any API.

m2049r · 2018-03-10T13:29:09Z

i suck at C++, but as far as i can tell, the wallet2 api uses the tools::wallet2 generate methods. which now behave differently.

EDIT: if this is true, it's now impossible to create the address.txt file through the api.

(i also noticed that the new parameter create_address_file is undocumented in the wallet2.h declarations, whereas the other parameters are)

…r new wallets Previously, a file containing the unencrypted Monero address was created by default in the wallet's directory. This file might pose as a privacy risk. The creation of this file is now opt-in and can be enabled by providing --create-address-file

leonklingele · 2018-03-10T15:28:46Z

@m2049r the wallet2 API can be found here: https://github.com/monero-project/monero/blob/e9f41e405f1f75533ef6e9d5adc76ba41dd29e8c/src/wallet/api/wallet2_api.h

(i also noticed that the new parameter create_address_file is undocumented in the wallet2.h declarations, whereas the other parameters are)

Good catch, updated.

m2049r · 2018-03-10T15:52:03Z

the wallet2 API can be found here

yes, and it uses the generate() methods changed in this PR effectively changing the api without the possibility for the same behaviour.

if you find the time, please comment on my original comment concerning testnet/mainnet discrepancies & possibility to create/remove address.txt after wallet creation.

moneromooo-monero · 2018-03-10T17:51:53Z

Ah, those are not meant to be stable. Nothing in the code is yet.

m2049r · 2018-03-11T07:46:44Z

even so - i don't understand the need for deliberately introducing a discrepancy between simplewallet and the wallet2 api functionality.

as there are at least three wallets based on the wallet2 api i don't feel it's a good idea to treat it as "unstable" by default (although it may be).
i was under the impression that @dEBRUYNE-1 and @stoffu agreed with me on this.

stoffu · 2018-03-11T09:33:06Z

The API is exposed through wallet2_api.h which is not changed in this PR. 3rd party developers shouldn’t need to worry about how the API calls get passed to the actual wallet2 code, as implemented in e.g. api/wallet.cpp.

IMO whether to generate address.txt or not is rather an internal design decision and not part of the API.

m2049r · 2018-03-11T09:50:00Z

an API is not merely a method signature - it's a contract as to what the method does (incl. side effects). the address.txt is the direct result of calling a create method and a caller may rely on that - like they rely on the keys file being created in a specific place. in particular, monerujo does exactly this. as you can see in the comments to this PR many users would like to see the public address without having to open their wallets. monerujo shows the address.txt info in the wallet list for this purpose (future plans include optionally removing the address.txt for more privacy as described in this PR).
IMO making this impossible from the API (and possible from simplewallet) is not a good design choice but, of course, i would accept it and shut up now.

moneromooo-monero · 2018-03-11T10:05:04Z

OK, so your problem is that the API from wallet2_api.h did NOT change, then. Patches welcome.

stoffu · 2018-03-11T13:56:13Z

src/wallet/wallet2.h

+     * \param  wallet_              Name of wallet file
+     * \param  password             Password of wallet file
+     * \param  viewkey              view secret key
+     * \param  spendkey             spend secret key


Looks like the address argument is also undocumented here (which is also the case for another overload below). Also, the ordering of spendkey and viewkey is reversed. These are minor issues, but maybe you can fix them as well while you're at it?

I'll open a separate PR to fix those comments.

Fixed with #3405

stoffu · 2018-03-11T14:01:13Z

@m2049r

an API is not merely a method signature - it's a contract as to what the method does (incl. side effects). the address.txt is the direct result of calling a create method and a caller may rely on that

That's a valid point. Then, maybe api/wallet.cpp should be updated such that every call m_wallet->generate(...) is passed true as the parameter create_address_file. This way, the wallet's behavior stays the same as before.

moneromooo-monero · 2018-03-11T14:05:52Z

That will cause the GUI wallet to leave these, which would defeat most of the point of this patch.

stoffu · 2018-03-11T14:17:18Z

I was somehow assuming that this patch was for the CLI users. If the ability to opt-in for the creation of address.txt is to be available for the GUI as well, then the API also needs to be changed. I'm not sure if that's outside the scope of this PR, though.

moneromooo-monero · 2018-03-11T14:25:41Z

I was assuming the intent was to prevent people from unknowingly leaking their address. If so, GUI users are more likely to not realize they do. Not a huge leak, mind.

fluffypony

Reviewed

649a1b7 wallet2 / simplewallet: Must opt-in to create '.address.txt' files for new wallets (Leon Klingele)

leonklingele force-pushed the nuke-address-file branch from f1469d3 to 7a186cd Compare February 26, 2018 10:40

stoffu reviewed Feb 26, 2018

View reviewed changes

leonklingele force-pushed the nuke-address-file branch from d1162a4 to 5ae770b Compare February 28, 2018 08:50

stoffu approved these changes Mar 1, 2018

View reviewed changes

moneromooo-monero approved these changes Mar 1, 2018

View reviewed changes

fluffypony approved these changes Mar 5, 2018

View reviewed changes

leonklingele force-pushed the nuke-address-file branch from 5ae770b to 328f759 Compare March 5, 2018 23:41

leonklingele force-pushed the nuke-address-file branch from 328f759 to 649a1b7 Compare March 10, 2018 15:28

stoffu reviewed Mar 11, 2018

View reviewed changes

fluffypony approved these changes Mar 14, 2018

View reviewed changes

fluffypony merged commit 649a1b7 into monero-project:master Mar 14, 2018

fluffypony added a commit that referenced this pull request Mar 14, 2018

Merge pull request #3315

3fed788

649a1b7 wallet2 / simplewallet: Must opt-in to create '.address.txt' files for new wallets (Leon Klingele)

cryptochangements34 mentioned this pull request May 30, 2018

wallet2: only try to remove temp address file if it was created #3881

Closed

wallet2: No longer create '.address.txt' files for non-testnet wallets #3315

wallet2: No longer create '.address.txt' files for non-testnet wallets #3315

Conversation

leonklingele commented Feb 25, 2018

iDunk5400 commented Feb 25, 2018

leonklingele commented Feb 25, 2018

iDunk5400 commented Feb 25, 2018

arnuschky commented Feb 26, 2018

moneromooo-monero commented Feb 26, 2018

arnuschky commented Feb 26, 2018

leonklingele commented Feb 26, 2018

stoffu Feb 26, 2018 • edited

Choose a reason for hiding this comment

moneromooo-monero Feb 26, 2018

Choose a reason for hiding this comment

leonklingele Feb 26, 2018

Choose a reason for hiding this comment

stoffu Feb 26, 2018 • edited

Choose a reason for hiding this comment

hyc Feb 26, 2018

Choose a reason for hiding this comment

arnuschky Feb 26, 2018

Choose a reason for hiding this comment

leonklingele commented Feb 27, 2018

stoffu commented Feb 28, 2018

Gingeropolous commented Mar 2, 2018

hyc commented Mar 2, 2018

fluffypony left a comment

Choose a reason for hiding this comment

fluffypony commented Mar 5, 2018

leonklingele commented Mar 5, 2018

ghost commented Mar 6, 2018

m2049r commented Mar 10, 2018

moneromooo-monero commented Mar 10, 2018

m2049r commented Mar 10, 2018 • edited

leonklingele commented Mar 10, 2018

m2049r commented Mar 10, 2018

moneromooo-monero commented Mar 10, 2018

m2049r commented Mar 11, 2018

stoffu commented Mar 11, 2018

m2049r commented Mar 11, 2018

moneromooo-monero commented Mar 11, 2018

stoffu Mar 11, 2018

Choose a reason for hiding this comment

leonklingele Mar 11, 2018

Choose a reason for hiding this comment

leonklingele Mar 14, 2018

Choose a reason for hiding this comment

stoffu commented Mar 11, 2018

moneromooo-monero commented Mar 11, 2018

stoffu commented Mar 11, 2018

moneromooo-monero commented Mar 11, 2018

fluffypony left a comment

Choose a reason for hiding this comment

stoffu Feb 26, 2018 •

edited

stoffu Feb 26, 2018 •

edited

m2049r commented Mar 10, 2018 •

edited