make cli_wallet rpc logging work

[jmjatlanta]

Fixes #1149 and #1567.

Although the cli_wallet said it was logging RPC messages to a log file, it was not. This change fixes the problem.

This fix also names the log file directory slightly differently to prevent conflicts (i.e cli_wallet is run along with witness_node).

[abitmore]

Just a thing: please avoid logging passwords, as well as private keys. That means we should not directly log commands like "unlock" and "import_key" and probably a few others.

[oxarbitrage]

Also, unsure if it is related but it seems that we loss color when the logger is working or maybe just me ?

I see for example in red the following sentence in previous versions when starting the wallet:

1323695ms th_a main.cpp:200 main ] wdata.ws_user: wdata.ws_password:

While in this version i see all in just white font.

[jmjatlanta]

Also, unsure if it is related but it seems that we loss color when the logger is working or maybe just me

Yes, you were right. Color added.

[jmjatlanta]

@oxarbitrage Moved the logging to a separate method. Let me know if you like that better.

[abitmore]

I still think we should avoid logging passwords and private keys.

[cogutvalera]

I still think we should avoid logging passwords and private keys.

Absolutely agree with you !

[jmjatlanta]

I still think we should avoid logging passwords and private keys.

I totally agree with you. With the current code, I can find nowhere that private keys or passwords will be logged. Some results do display keys (i.e. suggest_brain_key), but obviously they need to be displayed. However the rpc log file does not contain sensitive data, as things like passwords and private keys never go across the wire. I have tested with set_password, unlock, import_key.

[abitmore]

@jmjatlanta so what are logged in the files? Do you have a sample? Thanks.

[jmjatlanta]

@jmjatlanta so what are logged in the files? Do you have a sample? Thanks.

https://pastebin.com/pL0h2DsX

[abitmore]

So it's actually logging communications between the wallet and the node. IMHO it's not that useful (although still useful under some circumstance) since it's already done in the node. What we need for wallet is to log communications between a client and the wallet when the wallet is accepting RPC connections.

[jmjatlanta]

IMHO it's not that useful (although still useful under some circumstance) since it's already done in the node. What we need for wallet is to log communications between a client and the wallet when the wallet is accepting RPC connections.

I agree, but believe it is out of the scope of this issue. I will create a new issue.

[pmconrad]

@jmjatlanta status?

[jmjatlanta]

I believe this PR to be complete, albeit not very useful. This fixes a bug whereby logging was set up but not used.

A more useful log is to record the communication between a client and the wallet. That has been moved to #1570

[abitmore]

Actually this will fix #1567. It's still useful.

[pmconrad]

To make this even more useful it would be good to add command line options for specifying default and rpc log levels and rpc log file.